Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Breaking change in module variable assignment #56933

Open
MilesCranmer opened this issue Jan 1, 2025 · 5 comments
Open

Breaking change in module variable assignment #56933

MilesCranmer opened this issue Jan 1, 2025 · 5 comments
Labels
regression 1.11 Regression in the 1.11 release

Comments

@MilesCranmer
Copy link
Member

MilesCranmer commented Jan 1, 2025

I'd like to raise a concern about what appears to be a breaking change in Julia 1.11 regarding module variable assignment syntax:

# Julia 1.11
julia> Main.x = 1
ERROR: Global Main.x does not exist and cannot be assigned. Declare it using `global` before attempting assignment.
Stacktrace:
 [1] setproperty!
   @ ./Base.jl:63 [inlined]
 [2] setproperty!(x::Module, f::Symbol, v::Int64)
   @ Base ./Base.jl:61
 [3] top-level scope
   @ REPL[1]:1

# Previous behavior in 1.10
julia> Main.x = 1
1

While I understand from #54607 that having to declare Main.eval(Expr(:global, :x)) is now required, this change has caused immediate downstream impacts in the ecosystem. For example, PythonCall.jl/juliacall is experiencing breaks: JuliaPy/PythonCall.jl#582. I have written various educational material in PyJulia/juliacall that have relied on this syntax (where the setproperty! acts as a translation layer to Julia bindings) and those are now broken. This was also used as a mechanism to declare Python functions in Julia – JuliaPy/PythonCall.jl#394

Given that this modifies long-standing and very simple behavior that is used often in scripts (which are unlikely to show up in PkgEval), would it be possible to reconsider this as a breaking change more appropriate for a major version bump (i.e., Julia 2.0), or maybe do a soft deprecation for a few minor versions to give users warning?

@MilesCranmer
Copy link
Member Author

MilesCranmer commented Jan 1, 2025

Regarding @Keno's comment in the commit from #54678

We'll see what pkgeval says. If use is extensive, we may want to consider a softer removal strategy.

Could we maybe try doing a softer Base.depwarn on this for a couple of major versions (e.g., 1.11 and 1.12), with a helpful message for how-to-update, before a full removal? I do think this syntax is used more often in scripts than in packages so PkgEval likely misses the full picture of impact.

@Keno
Copy link
Member

Keno commented Jan 4, 2025

First, sorry that I missed your packages when I went around fixing this in the package ecosystem.
With regards to the deprecation, I don't think it's a good idea at this point. It would make this behavior inconsistent across the 1.11.x patch series (which has been out for 3 months) and would require additional compiler support. Very admittedly, the missing error check in this feature was a big oopsie, but I think a deprecation at this point would just make the mess bigger. This behavior was in the system for about a year, which admittedly is an annoyingly long time, but it also means that any scripts relying on it would have been written within the past year at most.

On a side note, I would also strongly advise reconsidering the automatic introduction of globals in Julia modules from the python side also. Following the 1.12 binding age partition (which has not landed yet), your users will likely run into a significant number of world age confusions. I understand that python users are used to being able to do this kind of thing, but it's not idiomatic in Julia.

@MilesCranmer
Copy link
Member Author

MilesCranmer commented Jan 5, 2025

Thanks for the reply and for acknowledging the oversight. I appreciate the work that went into fixing packages in the ecosystem.

My main concern about how the change was implemented:

The current error message makes no mention that this syntax used to work and was intentionally changed. This is problematic because:

  1. Users will waste time debugging what appears to be their own mistake. It's such a basic thing to do a variable assignment that even for me it was perplexing where the error was coming from, until I realised the syntax itself had broken.
  2. Part-time Julia users may conclude Julia is unstable when their working scripts suddenly break. I'm deeply involved in the ecosystem so I investigated the root cause, but most users won't.

My proposed solution is either:

  • Keep the functionality through 1.12 with a clear deprecation warning explaining the change and how to update, OR at minimum:
  • Update the error message to acknowledge this syntax changed in Julia 1.11, and provide clear migration guidance. Right now the error seems to suggest that this syntax never worked (or there is some other reason it isn't working), which is part of why it's so confusing to debug. And note that the syntax works in LTS Julia - we're stuck supporting code that can use it for several years at this point whether we like it or not.

Regarding PkgEval and GitHub-based fixes, I quote Goodhart's law: "When a measure becomes a target, it ceases to be a good measure." While PkgEval and GitHub are valuable for exploring stability of new updates, and helping people make patches, it shouldn't be relied on as a primary metric for evaluating breaking changes. Proprietary Julia libraries in production environments and industry settings will never appear in PkgEval – that code will just break. End-user code - written by people who are just using Julia as a day-to-day programming language – will just break.

Breaking changes need to consider this broader ecosystem. 1 year is a long time for software development so I really think it needs more warning. Not to mention some people will stick to LTS Julia for years before upgrading.

(Regarding future changes to globals and world age, thanks for pointing this out – when you implement these changes, please provide warning messages for at least a few versions beforehand if possible. Breaking changes are inevitable, but they should come with clear guidance on how to update code.)


Regarding the Python package in particular - We have a patch ready for PythonCall.jl (JuliaPy/PythonCall.jl#583), but I think it's important to provide some context on why we plan to keep it on the Python user side: this type of assignment has been fundamental to the Python<->Julia ecosystem for years, long before my time, and before it was apparently added to Julia itself. This syntax even appears on the quickstart page of pyjulia by @tkf (pyjulia is 8+ years old though I'm not sure when this feature was added):

Image

While pyjulia implemented this functionality manually and didn't rely on Julia's module variable assignment directly, newer packages like juliacall used this syntax when it became available in Julia – which is why it broke code for the ecosystem. (And juliacall has the unfortunate feature of automatically updating to the newest Julia version (which we should probably change) – meaning code is currently breaking without the user even changing anything)

Keno added a commit that referenced this issue Jan 5, 2025
As requested in #56933, acknowledge that this error was missing in Julia 1.9 and 1.10
and provide a reference to the issue so that people have an easier starting point.
@Keno
Copy link
Member

Keno commented Jan 5, 2025

I completely understand the frustration, but these things are tough calls. We're not perfect, so mistakes happen. In every release there are probable two or three dozen error paths that get added or deleted that were accidentally present or absent. Those cases are then a question of pragmatism. In some cases, such as this one, the mistake is problematic enough that it needs to be immediately corrected. In others, it's annoying but not serious so we leave it. We do use PkgEval to assess the situation as part of that pragmatic evaluation. We do in general need better mechanisms to handle these kinds of situations (e.g. #54903 and related, although we may not have used it in this situation) - our practice in this regard hasn't really changed, but the ecosystem has gotten a lot larger, so the frequency of it causing pain for our users has increased. I've submitted #56956 to adjust the error message to leave a reference. Again, I don't think we can change this behavior on 1.11 retroactively. At most, we could add the deprecation on 1.10, but our historical practice, has not been to not backport such things.

@MilesCranmer
Copy link
Member Author

Thanks, understandable. The error message is a good start - I will leave comments on it to suggest some tweaks.

Pragma strict sounds awesome by the way. You should check out the API in DispatchDoctor.jl for a local scoping approach to strictness which could be borrowed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression 1.11 Regression in the 1.11 release
Projects
None yet
Development

No branches or pull requests

3 participants