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

suggestion: always use PythonBundle instead of PythonPackage #15639

Open
smoors opened this issue Jun 8, 2022 · 7 comments
Open

suggestion: always use PythonBundle instead of PythonPackage #15639

smoors opened this issue Jun 8, 2022 · 7 comments
Labels
Milestone

Comments

@smoors
Copy link
Contributor

smoors commented Jun 8, 2022

for Python packages we use easyblock PythonPackage, unless it's more than 1 package, then we use PythonBundle.

this is annoying:

  • you always have to remember which one to use and how to use it
  • often when you update a PythonPackage, some additional packages must be added, so you have to change to PythonBundle anyway

so my suggestion: why not always use PythonBundle from the start? is there any downside?

see also #15491

@smoors smoors added the change label Jun 8, 2022
@boegel boegel added this to the 4.x milestone Jun 8, 2022
@boegel
Copy link
Member

boegel commented Jun 8, 2022

As far as I know from the top of my head, the installation of an easyconfig that uses PythonBundle with a single extension, or using PythonPackage directly, should be identical, both in terms of contents of the installation directory and the module file.

@smoors Are you up for verifying that's indeed the case first, before we consider actively recommending (and maybe even requiring it in PRs to the central easyconfigs repo) to use only PythonBundle even if there's only a single Python package being installed?

I guess that would be a bit awkward, but there's no technical reason not to do so that I can think of...

@smoors
Copy link
Contributor Author

smoors commented Jun 9, 2022

here are the differences in the module file. there are 3 main differences:

  • with PythonBundle, the package is registered as an Extension that can be searched for with Lmod, which I consider an improvement
  • with PythonBundle, the lib path is appended to LIBRARY_PATH. not sure why this is needed, but I guess it doesn't hurt
  • with PythonBundle, EBEXTSLISTSKLEARNMINPANDAS is defined
--- /scratch/brussel/vo/000/bvo00005/vsc10009/ebtest/sklearn-pandas/modules/all/sklearn-pandas/2.2.0-foss-2021b.lua     2022-06-09 13:54:16.486328000 +0200
+++ /scratch/brussel/vo/000/bvo00005/vsc10009/ebtest/sklearn-pd/modules/all/sklearn-pandas/2.2.0-foss-2021b.lua 2022-06-09 13:46:04.393136000 +0200
@@ -10,6 +10,11 @@
 More information
 ================
  - Homepage: https://github.com/scikit-learn-contrib/sklearn-pandas
+
+
+Included extensions
+===================
+sklearn-pandas-2.2.0
 ]==])
 
 whatis([==[Description: 
@@ -18,8 +23,9 @@
 columns to transformations, which are later recombined into features.]==])
 whatis([==[Homepage: https://github.com/scikit-learn-contrib/sklearn-pandas]==])
 whatis([==[URL: https://github.com/scikit-learn-contrib/sklearn-pandas]==])
+whatis([==[Extensions: sklearn-pandas-2.2.0]==])
 
-local root = "/scratch/brussel/vo/000/bvo00005/vsc10009/ebtest/sklearn-pandas/software/sklearn-pandas/2.2.0-foss-2021b"
+local root = "/scratch/brussel/vo/000/bvo00005/vsc10009/ebtest/sklearn-pd/software/sklearn-pandas/2.2.0-foss-2021b"
 
 conflict("sklearn-pandas")
 
@@ -36,9 +42,11 @@
 end
 
 prepend_path("CMAKE_PREFIX_PATH", root)
+prepend_path("LIBRARY_PATH", pathJoin(root, "lib"))
 setenv("EBROOTSKLEARNMINPANDAS", root)
 setenv("EBVERSIONSKLEARNMINPANDAS", "2.2.0")
 setenv("EBDEVELSKLEARNMINPANDAS", pathJoin(root, "easybuild/sklearn-pandas-2.2.0-foss-2021b-easybuild-devel"))
 
 prepend_path("PYTHONPATH", pathJoin(root, "lib/python3.9/site-packages"))
 -- Built with EasyBuild version 4.5.5.dev0-r8f014a8f1243272fef46c07b8a9341b1b86f6dbf
+setenv("EBEXTSLISTSKLEARNMINPANDAS", "sklearn-pandas-2.2.0")

@smoors
Copy link
Contributor Author

smoors commented Jun 9, 2022

in the install dir, the changes are very minimal:

  • as expected, bundle and pythonbundle easyblocks added to the easybuild/reprod/easyblocks dir
  • Lmod envars set in the easybuild/*-easybuild-devel file:
    • setenv("LMOD_REDIRECT", "no") # force the output of list, avail and spider to go to stdout instead of stderr
    • setenv("LMOD_EXTENDED_DEFAULT", "no") # disables loading partial module versions
    • setenv("LMOD_IGNORE_CACHE", "1")
  • metadata about the build dir changed in the .dist-info dir (harmless)
--- /scratch/brussel/vo/000/bvo00005/vsc10009/ebtest/sklearn-pandas/software/sklearn-pandas/2.2.0-foss-2021b/lib/python3.9/site-packages/sklearn_pandas-2.2.0.dist-info/direct_url.json>2022-06-09 13:53:52.512420000 +0200
+++ /scratch/brussel/vo/000/bvo00005/vsc10009/ebtest/sklearn-pd/software/sklearn-pandas/2.2.0-foss-2021b/lib/python3.9/site-packages/sklearn_pandas-2.2.0.dist-info/direct_url.json>2022-06-09 13:45:22.299471000 +0200
@@ -1 +1 @@
-{"dir_info": {}, "url": "file:///tmp/sklearnpandas/2.2.0/foss-2021b/sklearn-pandas-2.2.0"}
\ No newline at end of file
+{"dir_info": {}, "url": "file:///tmp/sklearnpandas/2.2.0/foss-2021b/sklearnpandas/sklearn-pandas-2.2.0"}
\ No newline at end of file
diff -ur /scratch/brussel/vo/000/bvo00005/vsc10009/ebtest/sklearn-pandas/software/sklearn-pandas/2.2.0-foss-2021b/lib/python3.9/site-packages/sklearn_pandas-2.2.0.dist-info/RECORD /scratch/brussel/vo/000/bvo00005/vsc10009/ebtest/sklearn-pd/software/sklearn-pandas/2.2.0-foss-2021b/lib/python3.9/site-packages/sklearn_pandas-2.2.0.dist-info/RECORD
--- /scratch/brussel/vo/000/bvo00005/vsc10009/ebtest/sklearn-pandas/software/sklearn-pandas/2.2.0-foss-2021b/lib/python3.9/site-packages/sklearn_pandas-2.2.0.dist-info/RECORD>-2022-06-09 13:53:52.514923000 +0200
+++ /scratch/brussel/vo/000/bvo00005/vsc10009/ebtest/sklearn-pd/software/sklearn-pandas/2.2.0-foss-2021b/lib/python3.9/site-packages/sklearn_pandas-2.2.0.dist-info/RECORD>-2022-06-09 13:45:22.302271000 +0200
@@ -4,7 +4,7 @@
 sklearn_pandas-2.2.0.dist-info/RECORD,,^M
 sklearn_pandas-2.2.0.dist-info/REQUESTED,sha256=47DEQpj8HBSa-_TImW-5JCeuQeRkm5NMpJWZG3hSuFU,0^M
 sklearn_pandas-2.2.0.dist-info/WHEEL,sha256=Z-nyYpwrcSqxfdux5Mbn_DQ525iP7J2DG3JgGvOYyTQ,110^M
-sklearn_pandas-2.2.0.dist-info/direct_url.json,sha256=DPNhpA3gZTy5m8eyLDhUVeZoIGkOxmnXUBBnQn2x4Tk,90^M
+sklearn_pandas-2.2.0.dist-info/direct_url.json,sha256=N3RwyHpoG2H3qm-1a53hFoz1bBBTj5GVRHclw9xulPU,104^M
 sklearn_pandas-2.2.0.dist-info/top_level.txt,sha256=qwEWyA_CKkNRd16IWPhIuKZbfaHg0bcCRp2urjCBbZ0,15^M
 sklearn_pandas/__init__.py,sha256=YxXBJOGjCK312lz46J0yaPqz5aMPV_ARMw3Jz_Sptzo,237^M
 sklearn_pandas/__pycache__/__init__.cpython-39.pyc,,^M

@boegel
Copy link
Member

boegel commented Jun 10, 2022

@smoors As briefly discussed during the EasyBuild conf call this week, I can see the annoyance with PythonPackage vs PythonBundle, but I'm also wondering how we should move forward if we agree on recommending to only use PythonBundle going forward (assuming there's agreement on that between maintainers, I would like to hear a couple of additional voices in here @easybuilders/easybuild-easyconfigs-maintainers...)

Shall we start making the easyconfigs test suite fail for easyconfigs that use PythonPackage directly, and ask to switch to PythonBundle instead?
If so, we should include the URL of a docs page that explains the motivation behind this...

@branfosj
Copy link
Member

I'd prefer us to use PythonBundle. However, I am concerned about how we make it happen. I do not agree with making it required - at least not quickly, as it adds an extra barrier to contributions.

@smoors
Copy link
Contributor Author

smoors commented Jun 12, 2022

we could also provide a script that automatically converts PythonPackage easyconfigs to PythonBundle?
or even run the script whenever a PythonPackage easyconfig PR is created?

@boegel
Copy link
Member

boegel commented Jun 22, 2022

If we make it required for the central easyconfig repository, we should only enforce it for easyconfigs being touched in new PRs, by adding a test to the easyconfigs test suite.
Before enforcing the use of PythonBundle, we should add a page to the documentation to explain why we're doing this, and providing some clear guidelines for to convert an easyconfig using PythonPackage to PythonBundle (or implement a script or even an option in EasyBuild itself to do the conversion)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants