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

Test lower compat bounds? #378

Open
oschulz opened this issue Jul 25, 2024 · 22 comments · Fixed by #379
Open

Test lower compat bounds? #378

oschulz opened this issue Jul 25, 2024 · 22 comments · Fixed by #379
Labels
test Related to the testing subpackage wontfix This will not be worked on

Comments

@oschulz
Copy link

oschulz commented Jul 25, 2024

The current DocStringExtensions = "0.9.3" makes DifferentiationInterface incompatible with some packages that are still stuck on DocStringExtensions v0.8. Would it be possible to relax the version bound?

@gdalle
Copy link
Owner

gdalle commented Jul 25, 2024

Oh yes definitely. I wonder why CompatHelper didn't pick it up. Care to open a PR?

@oschulz
Copy link
Author

oschulz commented Jul 25, 2024

I wonder why CompatHelper didn't pick it up

Because I wanted to extend it downwards to DocStringExtensions = "0.8, 0.9", not upwards. :-) But I can't find the package anymore that caused the trouble, maybe my dependencies are all v0.9-compatible now. I'll close this for now ...

@oschulz oschulz closed this as completed Jul 25, 2024
@oschulz
Copy link
Author

oschulz commented Jul 25, 2024

In any case, since DifferentiationInterface wil (hopefully) be wide used, maybe we could use https://github.com/julia-actions/julia-downgrade-compat to keep the dependency version bounds wide, but in a safe manner?

@oschulz oschulz reopened this Jul 25, 2024
@oschulz oschulz changed the title Relax DocStringExtensions version bound? Use julia-downgrade-compat? Jul 25, 2024
@adrhill
Copy link
Collaborator

adrhill commented Jul 25, 2024

Sounds like a great idea, thanks!

@gdalle
Copy link
Owner

gdalle commented Jul 25, 2024

While a good idea in principle, downgrade compat testing is more tricky than it looks because I perform Pkg operations inside the DI test suite. The DI test suite is split into one CI workflow per AD backend, and each of those workflows activates a custom environment like DI/test/Back/Diffractor/Project.toml. This is designed to avoid instantiating an environment with all backends at once, which is very tricky due to conflicts (especially on older versions of said backends).
The trouble with this method is that compat bounds of the DI package are not enforced in the environment activated on top of the test environment, which is why I had to explicitly bound Diffractor for instance. So I'm not sure how the GitHub action you refer to will handle that.

@adrhill
Copy link
Collaborator

adrhill commented Jul 25, 2024

each of those workflows activates a custom environment like DI/test/Back/Diffractor/Project.toml.

This could possibly be handled through the projects argument:
https://github.com/julia-actions/julia-downgrade-compat?tab=readme-ov-file#usage

# Comma-separated list of Julia projects to modify. The Project.toml files in all of
# these directories will be modified.
# Example: ., test, docs
# Default: .
projects: ''

@gdalle
Copy link
Owner

gdalle commented Jul 25, 2024

I don't think so because the individual Project.toml files don't store the compatibility bounds of AD backends for DI: that's stored in the main Project.toml file. So we would need a way to enforce these bounds in the stacked environment.

@gdalle
Copy link
Owner

gdalle commented Jul 25, 2024

Maybe we could add the packages of test/.../Project.toml to the current environment instead of activating a new one? Not sure if there is a command for that

@adrhill
Copy link
Collaborator

adrhill commented Jul 25, 2024

Could you point me to the location where the /test/Back environments get activated?

@gdalle
Copy link
Owner

gdalle commented Jul 25, 2024

function subtest(category, folder)
Pkg.activate(joinpath(@__DIR__, category, folder))
Pkg.instantiate()
@testset "$file" for file in filter(
endswith(".jl"), readdir(joinpath(@__DIR__, category, folder))
)
@info "Testing $category/$folder/$file"
include(joinpath(@__DIR__, category, folder, file))
end
Pkg.activate(TEST_ENV)
return nothing
end

@gdalle
Copy link
Owner

gdalle commented Jul 25, 2024

And a separate mechanism is at work for adding

because otherwise Pkg will try to grab it from the general registry

@adrhill
Copy link
Collaborator

adrhill commented Jul 25, 2024

To the best of my understanding, the way julia-downgrade-compat works is by simply rewriting the Project.toml files at the specified projects paths.

If we first activate DI/Project.toml before activating Pkg.activate(joinpath(@__DIR__, category, folder)), wouldn't the downgraded compat entries be enforced in the /test/BACK environments as well?

@gdalle
Copy link
Owner

gdalle commented Jul 25, 2024

No because compat bounds of environments deeper in the LOAD_PATH stack are not necessarily enforced:

Side note: activating one then the other is not enough, we need to push the former into the LOAD_PATH before activating the latter

@gdalle
Copy link
Owner

gdalle commented Jul 25, 2024

I thought it was cleaner to store this stuff in a Project.toml but maybe I can just write the list of packages explicitly. Let me open a quick PR.

@gdalle
Copy link
Owner

gdalle commented Jul 25, 2024

See #381, if tests pass on this one you can rebase #380 on it afterwards @adrhill

@adrhill
Copy link
Collaborator

adrhill commented Jul 25, 2024

#380 reveals some annoying properties of the julia-downgrade-compat action.
It looks like it rewrites the Project.toml to only keep the lowest compat entry, which often can't be properly resolved.

For example, the tests on #380 fail because SparseMatrixColorings has its DocStringExtensions compat entry set to v0.9:

ERROR: LoadError: Unsatisfiable requirements detected for package SparseMatrixColorings [0a514795]:
 SparseMatrixColorings [0a514795] log:
 ├─possible versions are: 0.1.0-0.3.5 or uninstalled
 ├─restricted to versions 0.3.0 by DifferentiationInterface [a0c0ee7d], leaving only versions 0.3.0
 │ └─DifferentiationInterface [a0c0ee7d] log:
 │   ├─possible versions are: 0.5.9 or uninstalled
 │   └─DifferentiationInterface [a0c0ee7d] is fixed to version 0.5.9
 └─restricted by compatibility requirements with DocStringExtensions [ffbed154] to versions: 0.1.0 or uninstalled — no versions left
   └─DocStringExtensions [ffbed154] log:
     ├─possible versions are: 0.4.6-0.9.3 or uninstalled
     └─restricted to versions 0.8.0 by DifferentiationInterface [a0c0ee7d], leaving only versions 0.8.0
       └─DifferentiationInterface [a0c0ee7d] log: see above

So these tests might be a headache in the future.

@gdalle
Copy link
Owner

gdalle commented Jul 25, 2024

Oh yeah I remember that discussion, essentially it tries to resolve all lower bounds at the same time.

https://discourse.julialang.org/t/psa-add-downgrade-ci-to-better-check-version-compatibility/110063/15?u=gdalle

@gdalle
Copy link
Owner

gdalle commented Jul 25, 2024

@adrhill can you modify #380 by

@adrhill
Copy link
Collaborator

adrhill commented Jul 25, 2024

undoing #379 so that Pkg installs v0.9 of DocStringExtensions

I bet that doing this will just reveal another dependency conflict.

I suggest we keep #379 (which @oschulz needs) and don't use julia-downgrade-compat until there is a way to automatically resolve an environment with lower compat bounds. We can't be the ones the manually resolve all lower bounds. Quoting from discourse:

trying to use [julia-downgrade-compat] on a big package with many deps just felt like a total waste of time. It needs to do something smarter than just pinning everything to the minimum versions.

@gdalle
Copy link
Owner

gdalle commented Jul 25, 2024

I bet that doing this will just reveal another dependency conflict.

Or maybe not, that's my hope.

If that's the only problem, gdalle/SparseMatrixColorings.jl#31 should be enough to fix it for good

@adrhill
Copy link
Collaborator

adrhill commented Jul 25, 2024

I bet that doing this will just reveal another dependency conflict.

Or maybe not, that's my hope.

If not, then that's pure luck.

We'll encounter the same problem of having to manually resolve lower compat bounds every time we need to modify lower bounds. And that's going to happen every time we require a new feature from a new release of any dependency.

@gdalle gdalle added the test Related to the testing subpackage label Jul 29, 2024
@adrhill
Copy link
Collaborator

adrhill commented Aug 1, 2024

While we should test lower compat bounds, julia-downgrade-compat doesn't seem to be the answer for DI. We tried using it in #380, but it currently requires manual resolving of dependencies, which should ideally be handled by Pkg rather than a human. This problem was discussed on Discourse as well.

A proposal to add down to Pkg can be found here: JuliaLang/Pkg.jl#1062
Until this is added, this issue will probably have to be left open.

@adrhill adrhill changed the title Use julia-downgrade-compat? Test lower compat bounds? Aug 1, 2024
@gdalle gdalle added the wontfix This will not be worked on label Sep 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Related to the testing subpackage wontfix This will not be worked on
Projects
None yet
3 participants