cmake: embed Python dependencies #47501
Conversation
pereman2
requested review from
nSedrickm and
melissa-kun-li
and removed request for
a team
pereman2
requested review from
tchaikov,
djgalloway and
epuertat
and removed request for
nSedrickm and
melissa-kun-li
|
@pereman2 apart from regular (old-fashioned) pip, we can explore modern incarnations of that, like pipenv, pip-compile or poetry. The idea of having the plain requirements defined, and later the whole list of pinned dependencies is closer to our (not so bad) experience with Dependabot supports the following formats for defining Python dependencies:
|
There was a problem hiding this comment.
Really nice PoC, @pereman2 !
My observations:
- We cannot assume that during
cmakethe system will have access to the internet, so we should act otherwise. In fact, that's already the assumption and whyinstall-deps.shscans the requirements.txt and prefetches those Python deps. - There are (at least) 2 possible approaches here:
- Downloading but not installing the packages (e.g.:
install-deps.shandmake-distrunningpip download) and thencmakeruns thepip installfrom these downloaded packages. - The approach already used for npm packages: Python packages are downloaded and installed to their locations in the Ceph repo, and then are tarballed and 'injected' in the package stage
- Downloading but not installing the packages (e.g.:
- We need to add the requirements lock file and a check that fails if there's a mismatch between the requirements and the lock file (
npm installand other tools implement this behaviour by default).
pereman2
force-pushed
the
pip-dep
branch
2 times, most recently
from
6b0ba8f
to
2016ea2
Compare
|
jenkins test make check |
src/pybind/mgr/requirements-mgr.txt
Outdated
| CherryPy==18.8.0 | ||
| pyjwt==2.4.0 | ||
| routes==2.5.1 | ||
| werkzeug==2.0.3 | ||
| saml==0.9.0 | ||
| setuptools==59.6.0 |
There was a problem hiding this comment.
Are these the latest or the current ones?
There was a problem hiding this comment.
I've updated:
cherrypy from 18.4.0 > 18.8.0
saml 1.9.0 > 0.9.0, looks like a downgrade but pip install has these versions: 0.2.0, 0.2.1, 0.3.0, 0.3.1, 0.3.2, 0.3.3, 0.4.0, 0.5.0, 0.6.0, 0.7.0, 0.8.0, 0.9.0 reported when I try to install 1.9.0
the rest of packages have the same issue as the two above, so I had to either upgrade or to point to a similar version
There was a problem hiding this comment.
Interestingly we've been using the wrong python package for a while: it's the python-saml, not the saml one.
There was a problem hiding this comment.
the commit message is practically empty. would be better to put the content of cover letter into the commit message.
but a bigger problem is, this change makes the process of introducing dashboard simpler, but it defeats the purpose of packaging. and make the life of downstream maintainer more difficult.
i've been working on facilitating the process by introducing the .requires files, for instance https://github.com/ceph/ceph/blob/main/debian/ceph-mgr-dashboard.requires . i understand this is far from a complete solution. but instead of reinventing the wheel, i believe a better solution is to work with the packaging machinery instead of vendoring python dependencies.
i see two approaches here
- to package dashboard as a proper python package. so the packaging tools is able to extract the dependencies from, for instance, egg-info.
- to use the homebrew scripts to generate the dependencies from
requirements.txtand insert them intoceph.spec.inanddebian/control. the.requiresfiles is an early attempt in this direction.
|
@pereman2 this should be quicker to test & troubleshoot without teuthology, just Shaman (or a local rpmbuild) + simple vstart (ceph-dev) cluster running on RPMs. Here you have an example. |
|
@pereman2: I think I got it: there's a missing |
|
looks like that worked for https://pulpito.ceph.com/cbodley-2023-03-10_17:38:32-rgw:verify-wip-pereman2-testing-2022-09-05-1259-distro-default-smithi/ the
|
great news, thanks @epuertat and @cbodley for the pointers. Those warnings are expected |
|
jenkins test make check |
|
@pereman2 have you had a chance to investigate the PR check failures? |
pereman2
force-pushed
the
pip-dep
branch
2 times, most recently
from
bdb03bd
to
8385317
Compare
|
@cbodley should be fixed now... It was a stupid mistake |
|
there is a failure in tox-mgr where some nested dependecies of folders are not added to requirements I thought would happen by installing a dependency. For example: if you do locally On the other hand, If I run |
for example it tries to import things like winreg which are windows specific and in the previous example |
Based on https://github.com/ronf/asyncssh#optional-extras and https://github.com/ronf/asyncssh/blob/8c0b42ca4fce32a4ee1979b20de85a7b7b84f80b/setup.py#L61-L69, those packages are extras, so they need to be explicitly installed via |
With these changes python dependencies are embedded inside a `__pypackages__` folder in pybind/mgr which will be preprended to the `PYTHONPATH` so that that folder is the first one checked for dependencies. ceph.spec.in is updated to disable installing dashboard dependencies by default and instead we provide a `--without embedded_pip_pkgs` option to disable embedded dependencies. New dependencies are added to pybind/mgr/requirements-mgr.txt and a lock file is generated with pip-tools through a tox command: `tox -e gen-mgr-requirements`. A script is introduced, in script/pypackages.sh, to generate the __pypackages__ folder which holds the dependencies and later used by the mgr to import them. Signed-off-by: Pere Diaz Bou <pdiazbou@redhat.com>
|
A note from bug scrub: this PR (and its related ticket) have been discussed during a CLT meeting. The outcome in our memory is that the Dashboard Folks we'll take care (please correct if I'm wrong). |
|
This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days. |
|
jenkins retest this please |
|
@ceph/dashboard: ping. |
|
@rzarzynski IMHO this is still a sensible approach... but not as urgent as when the CentOS 9 Python deps were blocking Reef. I'd keep it open, even if PEP 582 (local |
|
This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days. |
|
This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution! |
With these changes python dependencies are embedded inside a
__pypackages__folder in pybind/mgr which will be preprended to thePYTHONPATHso that that folder is the first one checked fordependencies.
ceph.spec.in is updated to disable installing dashboard dependencies by
default and instead we provide a
--without embedded_pip_pkgsoption to disable embeddeddependencies.
New dependencies are added to pybind/mgr/requirements-mgr.txt and a lock
file is generated with pip-tools through a tox command:
tox -e gen-mgr-requirements.A script is introduced, in script/pypackages.sh, to generate the pypackages folder which holds
the dependencies and later used by the mgr to import them.
Signed-off-by: Pere Diaz Bou pdiazbou@redhat.com
Contribution Guidelines
To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard cephadmjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume toxjenkins test windows