Skip to content

JLD2 extension take 2 #597

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

JLD2 extension take 2 #597

wants to merge 5 commits into from

Conversation

christiangnrd
Copy link
Member

@lassepe I accidentally messed up your PR (#596) so I'm reopening this. I'll try to make it pass tests but if you get it fully working before then feel free to reopen and I'll close this one.

I apologize for the inconvenience.

@christiangnrd
Copy link
Member Author

christiangnrd commented Jun 16, 2025

I got the tests to pass. However, I'm not sure this is the best way to deal with the new JLD2 requirement. Do we add a JLD2 dependency directly to this package's setup file so that the downstream packages require no changes, or do we add JLD2 to every downstream package's test Project.toml?

Also, @maleadt I removed the TestEnv stuff since TestEnv.activate() was causing issues, and it was actually calling Pkg.activate(), so it wasn't actually being used. That may not be what you actually want.

@maleadt
Copy link
Member

maleadt commented Jun 17, 2025

Removing TestEnv is fine if the alternative works.

Do we add a JLD2 dependency directly to this package's setup file so that the downstream packages require no changes, or do we add JLD2 to every downstream package's test Project.toml?

In the past, we've opted for adding them to the test Project.toml, as running Pkg operations (mutating the active environment) during testing is not great, and may affect versions of other packages installed. It's an annoying way to handle this, but I don't think we can avoid it.

@lassepe
Copy link

lassepe commented Jun 17, 2025

@christiangnrd, can you elaborate on why it is even necessary to add JLD2 manually via Pkg.add? My understanding would be that if JLD2 is part of the test/Project.toml then it should be automatically available during Pkg.test?

Edit: I see now what is going on: the buildkite pipeline runs the tests of the downstream page (e.g. CUDA) and that package itself ends up including GPUArrays.jl:test/testsuite.jl

https://github.com/JuliaGPU/CUDA.jl/blob/8b6a2a06c8f9d247081d7c92d8f8a82ab68153d5/test/setup.jl#L12

which then triggers the "missing JLD2` dependency error.

I guess without fundamentally reworking the test infrastructure (e.g. by pulling out the testsuit stuff into its own package), merging dependencies manually (as @christiangnrd ) is unavoidable. Perhaps a minor improvement over the current approach: rather than manually adding only JLD2, I would suggest to do something like:

for (uuid, pkg_info) in gpuarray_test_deps
           Pkg.add(PackageSpec(name=pkg_info.name, uuid=uuid, version=pkg_info.version))
end

@maleadt
Copy link
Member

maleadt commented Jun 17, 2025

This is for reverse-CI, executing e.g. CUDA.jl's tests, which import GPUArrays.TestSuite. If that contains JLD2-dependent tests, the CUDA.jl test project also needs to depend on JLD2.

@lassepe
Copy link

lassepe commented Jun 17, 2025

You beat me to it! :) See my edited comment above ☝️

@christiangnrd
Copy link
Member Author

I opened JuliaGPU/CUDA.jl#2792, JuliaGPU/OpenCL.jl#329, JuliaGPU/oneAPI.jl#507, and merged JuliaGPU/Metal.jl#606.

Once merged we can rerun tests and then probably good to go.

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

Successfully merging this pull request may close these issues.

3 participants