Rework of the JuliaPackage easyblock - #4146
Conversation
…n_package` as all packages in the environment will be installed
6ac2dc8 to
c325f44
Compare
|
Moved the final setting of
|
…dir` and using builddependencies
…ve them from the final extension list
…ipped or ignored
773b011 to
e100f57
Compare
|
Concerning the points of "re-usability" of deps I am unsure what is the best way. The reason being that if 2 packages define the same extension, the compile cache will could work or be broken depending on the load order of the packages from what i've seen (this is why i check for conflicts when rebuilding the manifest). The old attempted route of just using the dependencies just as a source of information for the packages to install (but build a completely new and isolated environment) also has downsides:
EDIT: added a PR for what i have concerning the aforementioned packages Pinging also @lexming since he was the original author of the easyblock to get feedback on these points and the new EB in general |
Why would we need a bundle then? Isn't this effectively saying: Use |
There are some packages that have optional features that are enabled by installing a subpackage. In that case i expected them to always be bundable without problems. Indeed i think the proper approach would be to only have Package, even in Python we sometime encounter trouble when extensions of one package shadow one of another which depending on version might or might not work |
I mentioned something like that in Slack recently as it seems like PythonBundle is/should be preferred but IMO it makes it too easy to add additional packages that might silently conflict with others. The suggestion was to enhance the CI checks to catch that. |
|
@lexming Your feedback on this would be very valuable |
| 'dirs': [pkg_dir], | ||
| } | ||
|
|
||
| exts_filter = " && ".join(exts_filter) |
There was a problem hiding this comment.
It seems odd that it is set outside the if, but only used in one branch.
Besides that this will likely be fully ignored. Maybe you meant to add this as a sanity check command only?
And why isn't it done for non-extensions, i.e. plain JuliaPackages?
There was a problem hiding this comment.
I do not understand what you mean, in the case of a Bundle the sanity_check_step is handling setting it's own exts_filter in case it is an extension or just checking for the expected dir if it is the main bundle.
Do not remember if i tested it for simple packages, but with this, in the same spirit as python we should always use bundles
There was a problem hiding this comment.
You set exts_filter outside the if but only use it inside. So why not set it inside, closer to where it is used?
For non-extensions I'm quite sure exts_filter won't be used so you need the custom_cmds above. See comment there
in the same spirit as python we should always use bundles
I still think this is counter-productive as bundles for a single package are awkward to write and invite adding more packages that would better be suited for own easyconfigs.
I'm referring to both syntax by having to put everything in a dict with lots of extra quoting and the restrictions on what extensions can do, e.g. single-source only
There was a problem hiding this comment.
You set exts_filter outside the if but only use it inside. So why not set it inside, closer to where it is used?
Can agree on this (same point as the other thread and solved)
There was a problem hiding this comment.
I still think this is counter-productive as bundles for a single package are awkward to write and invite adding more packages that would better be suited for own easyconfigs.
I mean that is a decision that was taken more broadly, do not think this is the right venue to try and change this
There was a problem hiding this comment.
Will still keep using lists, i think it is more readable than concatenating strings with the risk of missing spaces and such
What about using custom_commands for that? See my earlier comment on possibly using exts_filter from the extension instead of the parent for filtering. Then it would become an issue and I'd say it is cleaner anyway to separate the "does it exist" from the "does it work" check.
I mean that is a decision that was taken more broadly, do not think this is the right venue to try and change this
Not sure where the right venue is but I'd like to bring that up before applying it to other Bundle-Package-changes than already is
There was a problem hiding this comment.
Was not even that aware that exts_filter was called so because of the --skip (even then still bad name), but for what i can see it is in general used to define the sanity check to be performed on an extension.
What about using custom_commands for that?
I think twice you refered to a wrong quote so i am not sure i am getting what you mean.
I think the problem is that we do not want to run the commands themselves on the Bundle.
We either split the sanity check for bundles in JuliaBundle and for Package only in JuliaPackage or i do not see this working. That is not just trivial refactoring...
There was a problem hiding this comment.
but for what i can see it is in general used to define the sanity check to be performed on an extension.
I guess it was an after-thought to use it as part of the sanity check too, which made sense when the definition is/was(?) basically: Check if this package is installed
I think twice you refered to a wrong quote so i am not sure i am getting what you mean.
What I meant: Have exts_filter be the simple existence check (julia -e ...), which (I think) it was supposed to be, and move the other command(s) that checks for correctness to custom_commands. This way you don't have to care about how separating and joining commands and it is cleaner, to me at least.
I think the problem is that we do not want to run the commands themselves on the Bundle.
If I see this correctly for extensions you run _COMPILECACHE_CHECK once, otherwise for each Julia package in all dependencies.
That's what you do right now: The bundle runs it through custom_commands already, the package and extension through exts_filter
If all you change is moving exts_filter.append(_COMPILECACHE_CHECK % {'ext_name': self.name, 'grep_loc': self.name}) to custom_commands instead and always add that, the behavior will still be the same, won't it?
I would even set cfg[exts_filter] in the constructor, so it can be changed by derived classes and later be used instead of the parent/bundle easyconfig for --skip (if we do that change at some point)
There was a problem hiding this comment.
I have a feeling that for this to work properly
https://github.com/easybuilders/easybuild-framework/blob/a464f4fa70b8df3bb4f54976d31f0aae199923cb/easybuild/framework/extensioneasyblock.py#L191
should really be a elif not self.is_extension.
Not sure why we would run the Extension sanity check on the bundle.
This combined with a
if not self.is_extension:
self.sanity_check_load_module()at the start of JuliaPackage sanity check would solve the issue.
Not sure though if the first part of the fix would have unforseen consequences, the way code for bundles is written is way too entangled for me to predict (and at the very least i would not block this PR for this since it can be added as a feature later).
Co-authored-by: Alexander Grund <Flamefire@users.noreply.github.com>
…rk with `include-easyblocks-from-pr` Co-authored-by: Alexander Grund <Flamefire@users.noreply.github.com>
…g in a non-existent dir
|
I now think this approach might not be the best: The major issue is that a JuliaBundle needs to be a Julia package or things fail at least in the sanity check. I'm not fully sure if this is really inherited but IIRC Bundles cannot have sources themselves, so that doesn't make sense to treat it as a package. Because unless we enforce that JuliaPackages are only ever part of a JuliaBundle then relying on |
So the main reason is that this was how the package was structured in the beginning so when i started making changes i tried to keep the original structure. If the only problem is using
I'd be more in favor of hiding/disallowing to make use of |
| # https://pkgdocs.julialang.org/v1/api/#Pkg.offline | ||
| env.setvar('JULIA_PKG_OFFLINE', 'false' if online else 'true') | ||
| else: | ||
| errmsg = ( |
There was a problem hiding this comment.
How does this not cause an error in Julia <1.5? Shouldn't you at least only error when online == True?
Also download_pkg_deps doesn't avoid this error: If set to True the error will still happen when passed to this function. And pkgs_to_test_online also triggers this error unconditionally and without need.
And maybe also mention test_online
It is not only that. Also
It could be a shared method. Or if the main point is doing a single "install cmd" on the last extension that could be something done by the Bundle: Go over the extensions and on the last consecutive JuliaPackage issue an install command on it, e.g. by passing an appropriate parameter to a method of that extensions easyblock
Besides that I wanted to discuss that I would not go so far as assuming that every |
Summary of changes
is_test_dependencyEC paramjulia_debugEC paramJuliaPackagecan't really work with depedencies #4123)Pkg.instantiate()to install all extensions instead of installing them one by one withPkg.developmax_paralleloption)runtestto True for every extension that needs testing)--skip-test-step) and ignorable (--ignore-test-failure)test_onlineis set to trueMaybe TODO
JuliaPackagecan't really work with depedencies #4123) so if the dep order is switched w.r.t. the one at build time slow re-compilation might happenNotes
This PRs includes the changes from:
--sanity-check-onlyfor aJuliaBundle#4122JuliaPackage: Allow running tests in a dedicated test_step #4124Fixes:
JuliaPackagecan't really work with depedencies #4123AI usage