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

Support more system-site packages #36256

Merged
merged 32 commits into from
Sep 16, 2023

Conversation

orlitzky
Copy link
Contributor

@orlitzky orlitzky commented Sep 12, 2023

Add boilerplate for a few more python packages to be detected with --enable-system-site-packages.

Marked as critical because it repairs rpy2 system package on ubuntu/debian

@dimpase
Copy link
Member

dimpase commented Sep 12, 2023

On Debian for poetry_core to work, one needs to install python3-pep517. Otherwise e.g. building pkgconfig fails, as import poetry doesn't work.

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 14, 2023

Thanks, this works.
I think you can revert f85cc8a now

Our system package lists are generated with a special case that looks
for SAGE_PYTHON_PACKAGE_CHECK when spkg-configure.m4 is present. The
rpy2 package already had an spkg-configure.m4 relating to --disable-r,
but its spkg-configure.m4 did not use SAGE_PYTHON_PACKAGE_CHECK. As a
result, it was erroneously included in the package list.

We now use SAGE_PYTHON_PACKAGE_CHECK for the usual "check" phase of
SAGE_SPKG_CONFIGURE, which both allows the user to use rpy2 from the
system and fixes the package list.
@orlitzky
Copy link
Contributor Author

Thanks, this works. I think you can revert f85cc8a now

Rebased away.

@github-actions
Copy link

Documentation preview for this PR (built with commit 905419a; changes) is ready! 🎉

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 14, 2023

The struggles with poetry will go away after updates in #36129 and switching pkgconfig to a wheel package (#36181)

@tobiasdiez
Copy link
Contributor

On Conda the Conda provided rpy2 package seems to be rejected, see ci. Is this according to design?

Also there are a lot of errors that certain system packages couldn't be used.

@orlitzky
Copy link
Contributor Author

On Conda the Conda provided rpy2 package seems to be rejected, see ci. Is this according to design?

Also there are a lot of errors that certain system packages couldn't be used.

Nothing is being detected because --enable-system-site-packages was not passed to ./configure. Do those conda CI jobs try to force all system packages? If so, they should probably use the sage package-listing utilities to avoid listing python packages when --enable-system-site-packages was not given.

More generally though, we just enabled hundreds of system packages without much consideration for whether or not the popular versions of those packages actually work for sage. Conda (or any distro) won't be expected to work until someone tries to install all of these packages as system packages and then runs through the test suite and fixes all of the bugs they hit. It's still an experimental feature that, more than anything, lets us start fixing said bugs without having to jump through hoops.

@dimpase
Copy link
Member

dimpase commented Sep 15, 2023

I experimented with using this facility under Python 3.11.4 installed by pyenv.
I'm hitting a snag in form of which seem to tell something
along the lines of https://stackoverflow.com/questions/63958574/no-template-sub-directory-with-name-lab-found-in-the-following-paths

Here are more spkg-configure.m4 added:
0001-more-spkg-configure.m4-for-python-packages.patch.txt

[sagemath_doc_html-none] sage --docbuild --no-pdf-links reference/combinat inventory --no-prune-empty-dirs
[sagemath_doc_html-none] [repl     ] Exception occurred:
[sagemath_doc_html-none] [repl     ]   File "/home/dimpase/.pyenv/versions/3.11.5/lib/python3.11/site-packages/nbconvert/exporters/templateexporter.py", line 653, in get_template_names
[sagemath_doc_html-none] [repl     ]     raise ValueError(msg)
[sagemath_doc_html-none] [repl     ] ValueError: No template sub-directory with name 'script' found in the following paths:
[sagemath_doc_html-none] [repl     ]    /doesnotexist
[sagemath_doc_html-none] [repl     ]    /home/dimpase/work/software/sage/local/var/lib/sage/venv-python3.11/share/jupyter
[sagemath_doc_html-none] [repl     ]    /doesnotexist
[sagemath_doc_html-none] [repl     ]    /home/dima/.pyenv/share/jupyter
[sagemath_doc_html-none] [repl     ]    /usr/local/share/jupyter
[sagemath_doc_html-none] [repl     ]    /usr/share/jupyter
[sagemath_doc_html-none] [repl     ] The full traceback has been saved in /tmp/sphinx-err-mqe4c7d3.log, if you want to report the issue to the developers.
[sagemath_doc_html-none] [repl     ] Please also report this if it was a user error, so that a better error message can be provided next time.
[sagemath_doc_html-none] [repl     ] A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!
[sagemath_doc_html-none] Error building the documentation.

The latter can be fixed by

ln -s ~/.pyenv/versions/3.11.5/share/jupyter/nbconvert/ ~/work/software/sage/local/var/lib/sage/venv-python3.11/share/jupyter/
  • not sure how it shoud be properly fixed. It seems to be a bug on the Jupyter/Python border.

@orlitzky
Copy link
Contributor Author

* not sure how it shoud be properly fixed. It seems to be a bug on the Jupyter/Python border.

I can't really say. I'm not a notebook user anyway, but I also haven't even started to sort out the notebook issues with system site packages enabled. If I had to bet though, I'd go with: version mismatches. Our ipython and notebook are outdated, but many of their dependencies are missing bounds in install-requires.txt, so it's possible to wind up with an nbconvert much newer than the notebook itself. Regardless, I'm thinking it's probably a waste of time until the notebook-7.x upgrade takes place.

Here are more spkg-configure.m4 added: 0001-more-spkg-configure.m4-for-python-packages.patch.txt

I'm happy to have them but can we do it in a new ticket that isn't positively reviewed already? Note also that jupyter_packaging is being removed in #36267

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 15, 2023

Follow-up ticket please...

@dimpase
Copy link
Member

dimpase commented Sep 16, 2023

Follow-up ticket please...

Please see #36276

@vbraun vbraun merged commit cac16e2 into sagemath:develop Sep 16, 2023
24 of 44 checks passed
@mkoeppe mkoeppe added this to the sage-10.2 milestone Sep 16, 2023
@orlitzky orlitzky deleted the more-system-python-packages branch September 22, 2023 00:18
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 25, 2023
    
This is a continuation of sagemath#36256

- Fixes sagemath#36301
- Fixes https://groups.google.com/g/sage-
release/c/1wOBmhvNJqc/m/Jk14VAbjBAAJ (hence marked critical)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies


- sagemath#36296: to use an up to date Sphinx
- sagemath#36267: updated ipympl, etc

Also, a Jupyter/Python issue was uncovered there, which might need work.
    
URL: sagemath#36276
Reported by: Dima Pasechnik
Reviewer(s): Matthias Köppe, Michael Orlitzky
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 27, 2023
    
This is a continuation of sagemath#36256

- Fixes sagemath#36301
- Fixes https://groups.google.com/g/sage-
release/c/1wOBmhvNJqc/m/Jk14VAbjBAAJ (hence marked critical)

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies


- sagemath#36296: to use an up to date Sphinx
- sagemath#36267: updated ipympl, etc

Also, a Jupyter/Python issue was uncovered there, which might need work.
    
URL: sagemath#36276
Reported by: Dima Pasechnik
Reviewer(s): Matthias Köppe, Michael Orlitzky
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.

5 participants