-
-
Notifications
You must be signed in to change notification settings - Fork 396
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
DimensionalData extension #3383
Conversation
Co-authored-by: Oscar Dowson <[email protected]>
Project.toml
Outdated
test = ["DimensionalData", "Test"] | ||
|
||
[weakdeps] | ||
DimensionalData = "0703355e-b756-11e9-17c0-8b28908087d0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what happens with this in Julia 1.6? It just gets ignored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. extensions is a 1.9 feature after all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the options for Backwards compatibility is by Requires
or as a regular dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm okay if this is a Julia 1.9+ feature only. We don't need backwards compatibility for new features.
One issue is that the docs are currently build using Julia 1.6. So we'd need to update that, and then add some big compat warnings.
I guess a heads up: it's not certain that we'll merge an extension like this just yet. It's a pretty big change for JuMP, not because of this PR, but because it opens the door for others like it. But we have a monthly call on Thursday that I'll discuss this in. See also: #1350 |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #3383 +/- ##
==========================================
+ Coverage 98.06% 98.08% +0.01%
==========================================
Files 34 36 +2
Lines 4921 4957 +36
==========================================
+ Hits 4826 4862 +36
Misses 95 95
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this is pretty slick.
Before merging, we need a bunch more tests, nicer error messages, and some documentation.
But I'm in favor of adding this to JuMP. The main issue I see is a discoverability one. We'll likely need some careful documentation about how and when different extensions are loaded and what features they enable.
Some notes from the JuMP call
|
You can add |
The compat issue was this:
|
This is an interesting point about extensions in general for the ecosystem... we are making our dependency trees larger and that does have costs besides compile time. FWIW DimensionalData.jl is unlikely to do anything you cant support or change the basics of the dimarray interface much in the next few years. |
Based on my testing, it seems that the compat dependency issue is a hard requirement: (dd) pkg> st
Status `/private/tmp/dd/Project.toml`
[0703355e] DimensionalData v0.24.12
[4076af6c] JuMP v1.11.1 `~/.julia/dev/JuMP`
(dd) pkg> add DimensionalData@0.23
Resolving package versions...
ERROR: Unsatisfiable requirements detected for package DimensionalData [0703355e]:
DimensionalData [0703355e] log:
├─possible versions are: 0.1.0-0.24.12 or uninstalled
├─restricted to versions 0.24 by JuMP [4076af6c], leaving only versions: 0.24.0-0.24.12 or uninstalled
│ └─JuMP [4076af6c] log:
│ ├─possible versions are: 1.11.1 or uninstalled
│ └─JuMP [4076af6c] is fixed to version 1.11.1
└─restricted to versions 0.23 by an explicit requirement — no versions left Thus, I think we need to treat extensions as if we were adopting the dependency into JuMP proper. The lazy loading is almost an implementation detail, rather than an magic thing that gets loaded if the versions align. This makes me a lot more hesitant to add extensions for packages which are not version 1, or which have large dependency trees. In this case, DimensionalData is borderline, because it's pre-1.0, but it is fairly stable at v0.24.12, and it has a fairly reasonable set of dependencies: https://juliahub.com/ui/Packages/DimensionalData/Unppe/0.24.12?page=1 |
Closing in favor of #3413 |
adds
DimensionaData
as an extension, closes rafaqz/DimensionalData.jl#487