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

Add include_lazy_artifacts flag to create_app. #489

Closed
wants to merge 1 commit into from
Closed

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Jan 7, 2021

This pull request reconstitutes @rbvermaa's stab at #464 in https://github.com/RelationalAI-oss/PackageCompiler.jl/tree/rv-include-lazy. Rob notes

I tried to add an include_lazy_artifacts option [1], however, it is probably not a correct solution, as it ignores the situation where there is a Dict in the Artifact section. See first part of the if at https://github.com/RelationalAI-oss/PackageCompiler.jl/blob/rv-include-lazy/src/PackageCompiler.jl#L722 .

which I haven't looked into yet. But putting this pull request up to get the ball rolling :).

Co-Authored-By: Rob Vermaas [email protected]

@codecov-io
Copy link

codecov-io commented Jan 7, 2021

Codecov Report

Merging #489 (d5229b1) into master (e408e64) will increase coverage by 0.01%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #489      +/-   ##
==========================================
+ Coverage   82.65%   82.66%   +0.01%     
==========================================
  Files           2        2              
  Lines         392      398       +6     
==========================================
+ Hits          324      329       +5     
- Misses         68       69       +1     
Impacted Files Coverage Δ
src/PackageCompiler.jl 81.74% <60.00%> (-0.31%) ⬇️
src/juliaconfig.jl 90.47% <0.00%> (+2.67%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e408e64...d5229b1. Read the comment docs.

@Sacha0
Copy link
Member Author

Sacha0 commented Jan 7, 2021

Complete solution or no, CI seems happy! 😄

@Sacha0
Copy link
Member Author

Sacha0 commented Jan 26, 2021

Friendly bump. Thoughts? :)

@fguevaravas
Copy link

I think this is a great idea and will test this PR in my usage case and report back in a few days.

I have issues with apps generated with PackageCompiler.jl getting huge because every single artifact that might be remotely needed is downloaded. To give an example: compiling an app that uses FFTW.jl, downloads the whole Intel MKL and OpenMP even if the app never ends up using these (we're talking about 1.5GB of things that don't need to be packaged with app). Ditto for all the timezone data in TimeZone.jl

I am concerned about one thing though, what is the behavior if there is an artifact that is really needed by the app and it wasn't downloaded at compile time? Will the app download it and crash if it can't? If that is the case, it may be better to have a more granular control? Perhaps providing a list of artifacts to include/exclude?

@jlperla
Copy link

jlperla commented May 25, 2021

Sorry to bump this, but is this PR safe to use (or is there a good reason it hasn't been merged)? We have been bitten by the JuliaTime/TimeZones.jl#325 issue.

@Sacha0
Copy link
Member Author

Sacha0 commented May 25, 2021

We have been using this patch continuously for several months now, for what that's worth :). IIRC this pull request is waiting on formation of consensus on how best to move forward (with this or another approach to addressing the underlying issue).

@KristofferC
Copy link
Sponsor Member

Sorry, yes, we should clearly get this in.

@KristofferC
Copy link
Sponsor Member

#529 should hopefully do it.

@KristofferC KristofferC deleted the sv-rb-lazy branch September 16, 2021 08:46
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.

5 participants