Skip to content

__precompile__() on the main module and the extensions#96

Closed
camilodlt wants to merge 3 commits intomaltezfaria:mainfrom
camilodlt:main
Closed

__precompile__() on the main module and the extensions#96
camilodlt wants to merge 3 commits intomaltezfaria:mainfrom
camilodlt:main

Conversation

@camilodlt
Copy link
Copy Markdown
Contributor

Hello :),

I use DataFlowTasks.jl in one of my packages and I need to precompile it, however, I can't precompile it if it uses a package that has precompile(false) like the current version of DataFlowTasks.jl

I think I put all the code that is needed for making the precompilation work. I changed the PROJECT_ROOT global to a constant Ref{String} that is updated when the package is init.
I made a "hidden" method _get_dataflowtasks_root which just returns the value of the ref. I use that method everywhere where PROJECT_ROOT was used.

I ran all the tests on my computer and got all green.
I tested them on my machine which has the following versioninfo() and was able to precompile it:

julia> versioninfo()
Julia Version 1.10.5
Commit 6f3fdf7b362 (2024-08-27 14:19 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 12 × Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-15.0.7 (ORCJIT, skylake)
Threads: 1 default, 0 interactive, 1 GC (on 12 virtual cores)

@maltezfaria
Copy link
Copy Markdown
Owner

Thanks for the PR! It would be great to make DataFlowTasks pre-compilation friendly.

Any idea why the CI is failing on 1.11? The error is

ERROR: LoadError: InitError: MethodError: Cannot `convert` an object of type Nothing to an object of type String
The function `convert` exists, but no method is defined for this combination of argument types.

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.85%. Comparing base (bffb54f) to head (e184bf3).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #96      +/-   ##
==========================================
- Coverage   86.68%   81.85%   -4.83%     
==========================================
  Files           9        9              
  Lines         676      678       +2     
==========================================
- Hits          586      555      -31     
- Misses         90      123      +33     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@camilodlt
Copy link
Copy Markdown
Contributor Author

camilodlt commented Feb 13, 2025

Hello again, so it failed on 1.6.7 because global constants can not be typed on that version and it failed on 1.11 because we can not use pkdir inside the init in that version.

To solve it, I made a static check on the version and define a typed or untyped global constant accordingly. I left PROJECT_ROOT as a ref however.
For the second issue, the solution was to get the pkgdir at precompilation time which I don't think will pose a problem and, in a way, is what you were doing before.

I removed the __precompile__ statements from the extension as I'm not sure they are needed.

In my computer it passes tests on 1.6.7, 1.10.5, and 1.11.3

@maltezfaria
Copy link
Copy Markdown
Owner

Thanks for the fixes, the tests now pass!

I am trying to understand something rather basic here (have not played much with precompilation). IIUC, __precompile__(true) is the default, and at present when I do using DataFlowTasks a first time it does trigger precompilation (which throws no errors). So it is not clear to me why these changes are needed. In your original post you said that DataFlowTasks uses precompile(false), but is that something we currently do implicitly?

@camilodlt
Copy link
Copy Markdown
Contributor Author

I think that it turns out that I was using DataFlowTasks 0.1.0 in which there is a method overwrite, which prevents precompilation and I thought that a __precompile__() call would solve it. In that case, my problem relates to the dependencies which prevent the project to update to the 0.2.0 which is precompilable as is. In that case you can disregard my PR 😅

@maltezfaria
Copy link
Copy Markdown
Owner

I think that it turns out that I was using DataFlowTasks 0.1.0 in which there is a method overwrite, which prevents precompilation and I thought that a __precompile__() call would solve it. In that case, my problem relates to the dependencies which prevent the project to update to the 0.2.0 which is precompilable as is. In that case you can disregard my PR 😅

What dependencies are preventing the upgrade? Maybe DataFlowTasks is actually compatible with older versions of the packages listed in the Project.toml, in which case we could loosen the compat entry and tag a minor release so that you can update.

@camilodlt
Copy link
Copy Markdown
Contributor Author

I was able to update to 0.2.0 in my project by calling Pkg.add(Pkg.PackageSpec(;name="DataFlowTasks", version="0.2.0")) directly. I don't know why a simple ] update was not enough to update the package. I think that in that case I have to add a compat on my end requiring at least 0.2.0.

@maltezfaria
Copy link
Copy Markdown
Owner

I was able to update to 0.2.0 in my project by calling Pkg.add(Pkg.PackageSpec(;name="DataFlowTasks", version="0.2.0")) directly. I don't know why a simple ] update was not enough to update the package. I think that in that case I have to add a compat on my end requiring at least 0.2.0.

If for some reason you had a compat entry of DataFlowTasks pinned to 0.1 it won't automatically update to 0.2 because it is considered a breaking change. I recommend using CompatHelper to keep track of that!

Should we close this?

@camilodlt camilodlt closed this Feb 13, 2025
@camilodlt
Copy link
Copy Markdown
Contributor Author

I think the problem is that in my project I had Makie v0.22.1 which is not accepted in your compat which is checked even if I don't use the extras. Adding the specific version works by downgrading Makie, GLMakie in my case.

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.

2 participants