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

Replaced artifacts with JLL artifacts #19

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stemann
Copy link

@stemann stemann commented Sep 10, 2022

Using extended platform selection based on platform augmentation tags, i.e. https://pkgdocs.julialang.org/v1.7/artifacts/#Extending-Platform-Selection - adapted for use with JLL Artifacts.

Using extended platform selection based on platform augmentation tags, i.e. https://pkgdocs.julialang.org/v1.7/artifacts/#Extending-Platform-Selection - adapted for use with JLL Artifacts.
@codecov-commenter
Copy link

codecov-commenter commented Sep 10, 2022

Codecov Report

Base: 87.98% // Head: 88.67% // Increases project coverage by +0.68% 🎉

Coverage data is based on head (0cf858d) compared to base (f79d863).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #19      +/-   ##
==========================================
+ Coverage   87.98%   88.67%   +0.68%     
==========================================
  Files           3        3              
  Lines         383      371      -12     
==========================================
- Hits          337      329       -8     
+ Misses         46       42       -4     
Impacted Files Coverage Δ
src/highlevel.jl 91.02% <ø> (ø)
src/ONNXRunTime.jl 100.00% <100.00%> (ø)
src/capi.jl 87.41% <100.00%> (+0.54%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jw3126
Copy link
Owner

jw3126 commented Sep 10, 2022

Thanks a lot @stemann . I have never looked at augment_platform! I will read the docs, before I can give any meaningful feedback. Please feel free to ping me, should I forget to come back to this PR in the next week.

@jw3126
Copy link
Owner

jw3126 commented Sep 11, 2022

CUDA tests fail for me locally on this branch. I also discovered that my way of determining whether to run CUDA.jl tests does not work with recent CUDA versions and fixed that #20

@stemann
Copy link
Author

stemann commented Sep 11, 2022

Yes - it’s not even remotely ready.

CUDA binaries will not be available until JuliaPackaging/Yggdrasil#4386 is merged.

Current ONNXRuntime JLL provides “eager” artifacts. The CUDA EP PR will also make artifacts lazy.

The platform selection stuff is also only documented for Pkg/Julia v1.7, so that might also break v1.6 - and seems to be only/mostly relevant for “eager” artifacts.

@jw3126 jw3126 mentioned this pull request Mar 29, 2023
@stemann
Copy link
Author

stemann commented Aug 10, 2023

I stumbled a bit on the platform selection thing. I think what needs to be done is to have the ONNXRuntime JLL re-built using platform augmentation...

@jw3126
Copy link
Owner

jw3126 commented Aug 10, 2023

Very cool to hear from you again! If there is a clean way to make the jll artifacts work, that would be awesome.

@stemann
Copy link
Author

stemann commented Aug 10, 2023

Yeah - sorry - life happened :-) I'll try to find the time to finish this... (too...)

@jw3126
Copy link
Owner

jw3126 commented Oct 16, 2023

One general comment. The current approach we take to artifacts has many downsides, but one upside. It is quite simple, no need to compile anything we simply reuse binaries which onnxruntime and CUDA.jl provide. Also no usage of Artifacts.jl internals etc.

If we go with your approach, it would be important to me, that the thing stays easy to maintain.
Which means only use documented Artifacts.jl features, share artifacts with other packages (or onnxruntime) where possible and compile scripts that are simple to adapt to new versions.

@stemann
Copy link
Author

stemann commented Oct 17, 2023

Of course - it will of course get a little harder, as there is one more package (the JLL) to update and release, but it shouldn't get much harder.

I can ping you on PRs for updates of the JLL recipe, so you can easily see the path for upgrades.

@jw3126
Copy link
Owner

jw3126 commented Oct 17, 2023

Of course - it will of course get a little harder, as there is one more package (the JLL) to update and release, but it shouldn't get much harder.

I can ping you on PRs for updates of the JLL recipe, so you can easily see the path for upgrades.

Yeah I need to do an update myself.

@stemann
Copy link
Author

stemann commented Oct 22, 2023

I have just pushed JuliaPackaging/Yggdrasil#7561 which uses the same platform selection/augmentation block as CUDNN, i.e. the CUDA Runtime version is controlled via the CUDA_Runtime_jll version and/or local preferences.

It "also" builds the CUDA/CUDNN and TensorRT artifacts from source.

@stemann
Copy link
Author

stemann commented Dec 6, 2023

... still awaiting JuliaPackaging/Yggdrasil#7623 (needed for JuliaPackaging/Yggdrasil#7561)

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.

None yet

3 participants