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

change custom easyblock for Python to set pythonpath class variable to a fixed location relative to build path #3400

Closed
wants to merge 1 commit into from

Conversation

gkaf89
Copy link

@gkaf89 gkaf89 commented Jul 31, 2024

The pythonpath variable in the python.py EasyBlock is used to create temporary directories to store build artifacts. However, there are 2 problems with the current definition of the pythonpath instance field:

  • it uses the log directory to store build artifacts, and
  • the functions that use pythonpath do not accept a full path.

The build_path() function cannot be used instead of log_path() as the function provides a full path as well. Thus we opted to hard-code a relative path with respect to the build directory.

Issue: easybuilders/easybuild-easyconfigs#21078

@boegel boegel changed the title [bugfix] Set pythonpath to a fixed location relative to build path change custom easyblock for Python to set $PYTHONPATH to a fixed location relative to build path Jul 31, 2024
@boegel boegel added this to the release after 4.9.2 milestone Jul 31, 2024
@boegel boegel changed the title change custom easyblock for Python to set $PYTHONPATH to a fixed location relative to build path change custom easyblock for Python to set pythonpath class variable to a fixed location relative to build path Jul 31, 2024
@boegel
Copy link
Member

boegel commented Jul 31, 2024

@gkaf89 Thanks a lot for your in-depth analysis in easybuilders/easybuild-easyconfigs#21078, that's very helpful.

I'm not sure the proposed fix is correct though...
I think you're seeing a secondary effect of another problem that is likely to cause trouble somewhere else too.

I'll need some time to deep dive into this though, and fully understand what's going on.

@gkaf89
Copy link
Author

gkaf89 commented Aug 12, 2024

With the proposed modification the following file directory structure is created in the Python installation directory:

easybuild/python/sitecustomize.py

The easybuild normally contains the log files. If sitecustomize.py script is part of the log files, then the commit indeed has the potential to break any system that depends using sitecustomize.py from the log files.

However, based on definition of SITECUSTOMIZE, the file seems to target the installed software, but I cannot exclude that it is also needed by the files in the reprod directory of the logs.

If the sitecustomize.py is required by both the log files and the installed python software, maybe we need to extract out any functionality from the log files that depends on the software installation?

@Flamefire
Copy link
Contributor

@boegel Independent of #3493 this looks correct to me as it is merely a semantic change without any actual change as far as I can tell. I.e. the usage of log_path at that point isn't correct from a semantic POV

@gkaf89 gkaf89 changed the base branch from develop to 5.0.x November 20, 2024 19:02
@gkaf89 gkaf89 marked this pull request as ready for review November 20, 2024 21:56
@Flamefire
Copy link
Contributor

BTW: Can you change the title and description to make it fit better:

  • The path is relative to the install directory not the build path
  • The variable is not "used to create temporary directories to store build artifacts". There is nothing temporary about pythonpath
  • "the functions that use pythonpath do not accept a full path." -> "... expect a relative path"
  • The build_path() function -> Again nothing about the build path here, it is always used (more or less) relative to the install path

This would help better understanding the change some time later

The `pythonpath` variable was used to store python scripts in a
temporary location and later copy them in the installation path. The
scripts were stored in the path were log files are stored, `log_path()`,
in both the build and installation directories.

- Rename `pythonpath` to `python_script_dir_relative_path`.
- Use the more appropriate location `etc/python` to store the python
  scripts in the build and then installation directories.

Note that all functions handling the python script path accept relative
paths only. Currently, only the `sitecustomize.py` python environment
initialization script is stored in script directory.

Issue: easybuilders/easybuild-easyconfigs#21078
@gkaf89
Copy link
Author

gkaf89 commented Nov 24, 2024

The commit is reorganized according to the suggestions. All modification are made in a single commit so that they are easier to track. The underlying issue is that sitecustomize.py is an environment configuration script, and as such it should be stored under an etc directory in the installation directory of Python.

I was a bit confused about the role of sitecustomize.py as it was stored in the directory containing the EasyBuild installation logs.

@Flamefire
Copy link
Contributor

This now got superseded by getting rid of the path: #3514

This PR would break --module-only builds as the path changed. So I'd go with the other that takes care of that too

@boegel
Copy link
Member

boegel commented Dec 3, 2024

Indeed, let's go forward with the approach proposed in #3514, so closing...

@boegel boegel closed this Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants