-
Notifications
You must be signed in to change notification settings - Fork 203
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
prepend to $PYTHONPATH
or $EBPYTHONPREFIXES
in generated module files by automatically scanning for python site package directories
#4539
Conversation
a75d981
to
f5bf63d
Compare
$PYTHONPATH
or $EBPYTHONPREFIXES
by automatically scanning for python site package directories
0ce388e
to
8662f67
Compare
943ccd9
to
19a3b8d
Compare
dd17e4e
to
cd4d977
Compare
easybuild/framework/easyblock.py
Outdated
if len(python_paths) > 1 and not use_ebpythonprefixes: | ||
raise EasyBuildError('Multiple python paths requires EBPYTHONPREFIXES: ' + ', '.join(python_paths)) | ||
elif python_paths: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if len(python_paths) > 1 and not use_ebpythonprefixes: | |
raise EasyBuildError('Multiple python paths requires EBPYTHONPREFIXES: ' + ', '.join(python_paths)) | |
elif python_paths: | |
if python_paths: |
This is (as far as I have found, having tested just about every type of easyconfig i could find), which has an actual issue with the code right now
EasyBuild itself generates both:
/apps/Test2/software/EasyBuild/4.9.2/lib/python3.6/site-packages/
/apps/Test2/software/EasyBuild/4.9.2/lib64/python3.6/site-packages/
while the latter is empty, my glob'ing still picks it up, and having multiple PYTHONPATH's doesn't make any sense, this errors out.
There is technically nothing wrong here, those paths are both compatible with python3.6, so they would be fine to use.
I can just remove that extra check. Simplifies the code and you simply get what you ask for. If you have multiple paths for some reason, they are both added, no problem. This is just an extra check we added that were never cared about before, and it's in no way a requirement for this PR.
On the other hand, it's literally only EasyBuild that does this, and I think it shouldn't duplicate the installation directories. Perhaps it should be creating a lib <-> lib64 symlink like just about everything else?
I could also make the code more robust, checking if it's really the same python version etc...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this check for now, to keep this as simple as possible. This check could be introduced later if we want it (but we never checked for this in the past, so this change really has nothing to do with EBPYTHONPREFIXES, especially considering it actually only is problem when EBPYTHONPREFIXES isn't used)
740102c
to
47cd62b
Compare
I've made some simplifications:
|
$PYTHONPATH
or $EBPYTHONPREFIXES
by automatically scanning for python site package directories$PYTHONPATH
or $EBPYTHONPREFIXES
in generated module files by automatically scanning for python site package directories
Compared to #4496 it looks like this doesn't handle the case where easyblocks (and also easyconfigs?) add |
Correct, I don't override those if people have their own custom easyblocks or easyconfigs where they keep using PYTHONPATH (my accompanying PRs remove it from easybuild-easyblocks and easybuild-easyconfigs) |
Our sitecustomize.py only considers the lib/pythonx.xx/site-packages paths anyway, so we must not consider any more when adding the automatic paths.
8bb432f
to
ba28b96
Compare
Not only simplifies the code but is also the obviously correct way which i should have used from the start.
ba28b96
to
7e417d3
Compare
Instead, using absolute paths and then make them relative.
8a5682d
to
9813bc8
Compare
easybuild/framework/easyblock.py
Outdated
if path not in self.module_generator.added_paths_per_key[EBPYTHONPREFIXES]: | ||
lines.append(self.module_generator.prepend_paths(EBPYTHONPREFIXES, path)) | ||
else: | ||
for python_path in python_paths: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering: There can/should never be more than one here, can there? It would mean we'd have e.g. lib/python3.8
and lib/python2.6
and will add both to $PYTHONPATH which can't be right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit of history, went through a few iterations here, at some points i had lib/ and lib64/, which if not symlinked, could be both considered without being an error (and there was a few cases like that which falsely triggered an error), so I just got tired of dealing with that as well so I opted skip that error checking for now.
Since then, I've changed to only consider lib/ since that's the only thing EBPYTHONPREFIXES could handle anyway, so, yes, it's changed slightly. So yes, if someone makes a non PythonPackage/PythonBundle, which also does multi_deps for python, and they set force_python = True
for some reason, there would be multiple pythonpaths which maybe (possible likely) would break things (though breaking things isn't guaranteed if the actual python modules installed happens to work with both).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So when using $EBPYTHONPREFIX
, it's fine if there are multiple ones (there would be when using multi_deps
), but not when using $PYTHONPATH
, so we should make the latter an error.
I'll look into that, I'm working on this right now (implementing tests)
@@ -1105,15 +1105,19 @@ def filter_deps(self, deps): | |||
|
|||
return retained_deps | |||
|
|||
def dependencies(self, build_only=False): | |||
def dependencies(self, build_only=False, runtime_only=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we have either an "enum" dep_type=BUILD|RUNTIME|BOTH
or at least use include_build=True, include_runtime=True
because now we have a potentially "invalid" call: When both are True
Maybe we should at least rename to exclude_runtime, exclude_build
which would be mostly backwards compatible and setting both to True seems more reasonable to return nothing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's follow up on that in a separate PR, I want to get this PR merged ASAP.
Maybe open an issue for it, so we can tag it with 5.0
milestone.
…ectly by generated module file
…dule_pythonpath method
add test to verify that `$PYTHONPATH` or `$EBPYTHONPREFIXES` are set correctly by generated module file
…ATH or $EBPYTHONPREFIXES
…er if there's a real need for it
take into account `multi_deps` when determining whether to use `$PYTHONPATH` or `$EBPYTHONPREFIXES` + remove `force_pythonpath` easyconfig parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Merging, thanks @Micket ! |
Alternative approach, don't attempt to rewrite paths, instead generate them directly.
Will of course require some easyblocks (primarily pythonpackage and pythonbundle) to adapt by using these new options, though they should be trivial.
Note: very much a draft right now. Completely untested and probably forgot something important.
easyblock PR: easybuilders/easybuild-easyblocks#3343
easyconfig PR: easybuilders/easybuild-easyconfigs#20960