Arvados

21601-setuptools-git-deps @ 33b86b169d9427418e350f099a72c81707c13ede - developer-run-tests: #4090

Obviously the real test is in other Jenkins jobs, but I figure the code should be reviewed first, and if it looks good, then we can test other jobs before merging.

The way this works is that when we build/install a Python package from Git, instead of declaring dependencies on other Arvados packages as PACKAGE <= VERSION, we now declare them as PACKAGE @ file://REPO_PATH/PKG_PATH. This tells setuptools to get the package from that URL every time.

You can quickly test this for yourself by running from the branch, with some virtualenv activated:

% pip install arvados/sdk/cwl/
[…]
Successfully installed arvados-cwl-runner-2.8.0.dev20240316235340 arvados-python-client-2.8.0.dev20240316235340 crunchstat_summary-2.8.0.dev20240316234942

You can run this command multiple times, and it will always list those Arvados packages, because it always installs them from source. setuptools will never consider the requirement to already be satisfied as it did with version comparisons. Any time we install a package from Git, it will always stay in lockstep with all the other packages as they existed in that commit, which is what we've really wanted this whole time.

Note this works no matter what what pip's working directory is.

• All agreed upon points are implemented / addressed.
  • Should be, pending further testing, see above
• Anything not implemented (discovered or discussed during work) has a follow-up story.
  • So far so good
• Code is tested and passing, both automated and manual, what manual testing was done is described
  • See above
• Documentation has been updated.
  • N/A
• Behaves appropriately at the intended scale (describe intended scale).
  • No change
• Considered backwards and forwards compatibility issues between client and server.
  • All the functions that existed in arvados_version.py before this branch have backwards-compatible signatures
• Follows our coding standards and GUI style guidelines.
  • Yes

DEPRECATION: Source distribution is being reinstalled despite an installed package having the same name and version as the installed package. pip 21.1 will remove support for this functionality. A possible replacement is use --force-reinstall. You can find discussion regarding this at https://github.com/pypa/pip/issues/8711.

Is this going to be a problem? I read the linked issue (as well as a couple of other issues linked from that) and it's not clear if this affects our use case.

(cd ~/work/arvados/sdk/python/ && pip install -e .)
  Attempting uninstall: arvados-python-client
    Found existing installation: arvados-python-client 2.8.0.dev20240317180437
    Uninstalling arvados-python-client-2.8.0.dev20240317180437:
      Successfully uninstalled arvados-python-client-2.8.0.dev20240317180437
  Running setup.py develop for arvados-python-client
Successfully installed arvados-python-client-2.8.0.dev20240317180437

(cd ~/work/arvados/services/fuse/ && pip install -e .)
  Attempting uninstall: arvados-python-client
    Found existing installation: arvados-python-client 2.8.0.dev20240317180437
    Uninstalling arvados-python-client-2.8.0.dev20240317180437:
      Successfully uninstalled arvados-python-client-2.8.0.dev20240317180437
  Running setup.py install for arvados-python-client ... done
  Attempting uninstall: arvados-fuse
    Found existing installation: arvados-fuse 2.8.0.dev20240317180437
    Uninstalling arvados-fuse-2.8.0.dev20240317180437:
      Successfully uninstalled arvados-fuse-2.8.0.dev20240317180437
  Running setup.py develop for arvados-fuse
Successfully installed arvados-fuse-2.8.0.dev20240317180437 arvados-python-client-2.8.0.dev2024031718043

I don't think "arvados-python-client" is still editable after being reinstalled, I don't see an .egg-link file. That's not the end of the world (I can reinstall it as editable if I know that's happening). That might be the behavior that the deprecation warning is about -- it won't reinstall it in the future.

DEPRECATION: arvados-python-client is being installed using the legacy 'setup.py install' method, because it does not have a 'pyproject.toml' and the 'wheel' package is not installed. pip 23.1 will enforce this behaviour change. A possible replacement is to enable the '--use-pep517' option. Discussion can be found at https://github.com/pypa/pip/issues/8559

I don't remember seeing this warning before, but I could have just been ignoring it. Should we be doing something different here, like adding a dependency on wheel?

Currently, fpm_build_virtualenv_worker works by building a source dist and installing it into the virtualenv, and then telling fpm to make a package out of the virtualenv. (I think). Is the idea that packages built using fpm should install directly from the source tree? (We do need to figure that out as part of this branch).

not blocking: I noticed you switched parsing _version.py from a regex to using runpy. My gut instinct is to not make anything executable that doesn't have to be, but on the other hand that file is going to be imported at some point anyway so loading it as a module may give more consistent behavior.

not blocking: It's awkward that there are a bunch of copies of arvados_version.py but I don't think we can symlink them -- the reason for the current situation IIRC is that Python packaging ignores symlinks by default, at the time I didn't find an option to package them as regular files, and git treats hard links as regular files. It would be nice to have a check in run_tests that asserts all the copies of arvados_version.py are the same.

not blocking: I noticed the short_tests_only() function and went looking to see if we actually had tests marked as "short" or "slow" and it was not immediately clear we actually use it.

Peter Amstutz wrote in #note-6:

DEPRECATION: Source distribution is being reinstalled despite an installed package having the same name and version as the installed package. pip 21.1 will remove support for this functionality. A possible replacement is use --force-reinstall. You can find discussion regarding this at https://github.com/pypa/pip/issues/8711.

Is this going to be a problem?

I don't think so, and while I admit I don't feel 100% sure of that, I think if I'm wrong it'll be easy enough to fix with a flag like suggested, and/or reorganizing how run-tests.sh sets up its virtualenv. When I did my own suggested test of installing from source multiple times I got neither a warning or error, and:

% pip --version
pip 23.0.1 from /opt/arvados/pyclients/lib/python3.8/site-packages/pip (python 3.8)

So maybe they backed off this plan or something.

I don't think "arvados-python-client" is still editable after being reinstalled, I don't see an .egg-link file. That's not the end of the world (I can reinstall it as editable if I know that's happening).

IMO we should prioritize making it easier to write/maintain build tooling over supporting specific developer workflows like this, just because developers can hack together whatever they want however they want without as much concern for maintainability, backwards compatibility, etc. So yes, I think we should accept this and you should reinstall as editable as needed. I realize that might need to be a team decision though.

DEPRECATION: arvados-python-client is being installed using the legacy 'setup.py install' method, because it does not have a 'pyproject.toml' and the 'wheel' package is not installed. pip 23.1 will enforce this behaviour change. A possible replacement is to enable the '--use-pep517' option. Discussion can be found at https://github.com/pypa/pip/issues/8559

I don't remember seeing this warning before, but I could have just been ignoring it. Should we be doing something different here, like adding a dependency on wheel?

Adding wheel to the run-tests.sh venv seems easy enough and like a good idea. Long-term we should do #20311/#20723, but both of those are much bigger and tough to do on a 2.7.2 timeline.

Currently, fpm_build_virtualenv_worker works by building a source dist and installing it into the virtualenv, and then telling fpm to make a package out of the virtualenv. (I think). Is the idea that packages built using fpm should install directly from the source tree? (We do need to figure that out as part of this branch).

Correct, and yes, that's what I was trying to get at with "Obviously the real test is in other Jenkins jobs… we can test other jobs before merging." Since you asked and it seems like the branch is at least on acceptance track: build-packages-multijob: #4032

not blocking: I noticed you switched parsing _version.py from a regex to using runpy. My gut instinct is to not make anything executable that doesn't have to be, but on the other hand that file is going to be imported at some point anyway so loading it as a module may give more consistent behavior.

Yes. Also, I can't imagine any realistic threat model where an adversary can add arbitrary code to _version.py but not arvados_version.py, so I don't see any reason to be precious about it.

not blocking: It's awkward that there are a bunch of copies of arvados_version.py but I don't think we can symlink them -- the reason for the current situation IIRC is that Python packaging ignores symlinks by default, at the time I didn't find an option to package them as regular files, and git treats hard links as regular files.

I'll point out this branch at least improves the situation (IMO) by standardizing every copy and clearly separating package-specific data from logic. Current main has a bunch of copies which are slightly different from each other because there are package-specific differences to account for specific dependencies, etc.

It would be nice to have a check in run_tests that asserts all the copies of arvados_version.py are the same.

I agree, but which test suite should it be a part of?

not blocking: I noticed the short_tests_only() function and went looking to see if we actually had tests marked as "short" or "slow" and it was not immediately clear we actually use it.

I noticed this too. I'm happy to take on this cleanup but I was trying to limit the number of yaks shaved since this was on the critical path for 2.7.2. Again, we previously had this logic duplicated across multiple setup.py, this is at least an improvement in that now the logic is in a file that can be freely copied verbatim across the different packages that need it.

Brett Smith wrote in #note-7:

Currently, fpm_build_virtualenv_worker works by building a source dist and installing it into the virtualenv, and then telling fpm to make a package out of the virtualenv. (I think). Is the idea that packages built using fpm should install directly from the source tree? (We do need to figure that out as part of this branch).

Correct, and yes, that's what I was trying to get at with "Obviously the real test is in other Jenkins jobs… we can test other jobs before merging." Since you asked and it seems like the branch is at least on acceptance track: build-packages-multijob: #4032

This doesn't work as hoped because the shell builds an sdist and then installs that into the virtualenv, after the Git information has been lost. But I'm personally not aware of any reason we need to do that, so I think we could adapt by actually deleting some code from here to just install from source directly, and then adjust some of the metadata introspection to look in new locations. But might be worth checking with other folks first to see if someone else knows a reason I don't why the code currently works this way.

Brett Smith wrote in #note-7:

I don't think so, and while I admit I don't feel 100% sure of that, I think if I'm wrong it'll be easy enough to fix with a flag like suggested, and/or reorganizing how run-tests.sh sets up its virtualenv. When I did my own suggested test of installing from source multiple times I got neither a warning or error, and:

[...]

So maybe they backed off this plan or something.

$ pip3 --version
pip 23.0.1 from /home/peter/work/scripts/venv3/lib/python3.11/site-packages/pip (python 3.11)

Did you check for the error installing arvados-python-client or one of the packages that depend on it like arvados-fuse?

IMO we should prioritize making it easier to write/maintain build tooling over supporting specific developer workflows like this, just because developers can hack together whatever they want however they want without as much concern for maintainability, backwards compatibility, etc. So yes, I think we should accept this and you should reinstall as editable as needed. I realize that might need to be a team decision though.

I'm fine with this, I just wanted to note it.

Adding wheel to the run-tests.sh venv seems easy enough and like a good idea. Long-term we should do #20311/#20723, but both of those are much bigger and tough to do on a 2.7.2 timeline.

Do mean just the testing environment or the package building environment as well?

Correct, and yes, that's what I was trying to get at with "Obviously the real test is in other Jenkins jobs… we can test other jobs before merging." Since you asked and it seems like the branch is at least on acceptance track: build-packages-multijob: #4032

Got it.

It would be nice to have a check in run_tests that asserts all the copies of arvados_version.py are the same.

I agree, but which test suite should it be a part of?

Make it its own test. It is the sort of thing that should be checked by jenkins but doesn't need to run every single time some other test runs.

I am not diving into implementation just yet because I need to do reviews first, but I have read enough that here's how I think we should update the packaging process:

• If we're building any Python packages, first build wheels of all of them in one directory, using setup.py bdist_wheel.
• Turn that directory into a repository with piprepo or similar
• Then when we build the distribution packages, install the wheels directly into the built virtualenv using pip install --extra-index-url=/repo/path. This way pip can resolve the dependencies itself, without having to duplicate this information in the build script.

This will solve the general problem of not having dependency information duplicated across setup.py and the build scripts. It will also make the build process more efficient: right now we build some of the dependent packages (especially the Python SDK) multiple times. This way we'll only do it once.

You can also think of it as basically just reordering the steps we have right now. Right now the fpm virtualenv function does "set up virtualenv, build Python package, install Python package, build distro package." I'm proposing changing the process to "build Python packages, set up virtualenv, install Python package, build distro package."

21601-setuptools-git-deps @ fab6a2603007e5ef60385148d09d51f3ed0f5b2c - build-packages-multijob: #4065 - Note rocky8 package upload is failing because of #21616. The build is successful, and fixes for the upload will have to happen in completely different places, so there's no reason to hold up this branch on it.

This version rebases on main, squashing some of the bugfix thrashing that was in the previous branch, and implements the wheel-based install strategy discussed above. I also removed the "install from Git" logic from arvados_version.iter_dependencies because (a) it didn't solve the problem I hoped it would and (b) because of the issues Peter identified earlier. We still switch to specifying development dependencies with ~= to set a minimum version and prevent pip from installing release versions to satisfy development requirements.

This is admittedly a big change to solve the immediate problem, but it makes some progress on #20723, and it removes several fiddly special cases from the build code. It's more generic now and should serve us better in the long term.

I probably would not have rewritten arvados_version.py if I had realized we would need the wheel-based install solution anyway. But since I did, we might as well keep it, the way it lets every package use the same code without any special-case logic is a win.

Adding wheel to the run-tests.sh venv seems easy enough and like a good idea. Long-term we should do #20311/#20723, but both of those are much bigger and tough to do on a 2.7.2 timeline.

Do mean just the testing environment or the package building environment as well?

Well, it should be in both places, but it's already in the build environment in main, so I only mean adding it to run-tests.sh. That's done in 55703cfd8ee34f76a230ff256f51367f4fe3f77e.

I agree, but which test suite should it be a part of?

Make it its own test. It is the sort of thing that should be checked by jenkins but doesn't need to run every single time some other test runs.

I'm still not 100% sure I follow. You mean add a new command/function to run-tests.sh that only checks this? So it would implicitly be run by run-tests-remainder?

Meant to add, testing I did in addition to Jenkins: built debian12 packages locally, eyeballed package listings to confirm they still looked right, confirmed that scripts still have the correct shebang line, confirmed that special case files (cwltool in a-c-r, systemd service is docker-cleaner) are still packaged correctly.

Brett Smith wrote in #note-11:

Make it its own test. It is the sort of thing that should be checked by jenkins but doesn't need to run every single time some other test runs.

I'm still not 100% sure I follow. You mean add a new command/function to run-tests.sh that only checks this? So it would implicitly be run by run-tests-remainder?

Yes.

    # A packaged development release should be installed with other
    # development packages built from the same source, but those...
    dep_ver, match_count = re.subn(r'\.dev(0|[1-9][0-9]*)$', '.dev0', version, 1)
    dep_op = '~=' if match_count else '=='
    for dep_pkg in PACKAGE_DEPENDENCY_MAP.get(PACKAGE_NAME, ()):
        yield f'{dep_pkg}{dep_op}{dep_ver}'

Just checking that I'm reading this right -- if the version of the package has 'dev' in it, then we add a dependency on '~= X.Y.Z.dev0' but if the version doesn't have 'dev' in it then we have an exact dependency on the same version '== X.Y.Z', is that right?

https://packaging.python.org/en/latest/specifications/version-specifiers/#compatible-release

If I read this right, "~= X.Y.Z.dev0" means

= X.Y.Z.dev0, X.Y.Z.*

But I'm a little fuzzy on whether it is " X.Y.Z.*" or "== X.Y.*" -- is "X.Y.Z.dev0" normalized as "X.Y.Z.0dev0" ?

So which would match any development package in the X.Y.Z series or subsequent X.Y.Z release?

Anyway, this LGTM.

Peter Amstutz wrote in #note-14:

Brett Smith wrote in #note-11:

Make it its own test. It is the sort of thing that should be checked by jenkins but doesn't need to run every single time some other test runs.

I'm still not 100% sure I follow. You mean add a new command/function to run-tests.sh that only checks this? So it would implicitly be run by run-tests-remainder?

Yes.

Testing on developer-run-tests: #4122 just to confirm it actually ran (it did), will merge as long as I didn't break anything else.

Just checking that I'm reading this right -- if the version of the package has 'dev' in it, then we add a dependency on '~= X.Y.Z.dev0' but if the version doesn't have 'dev' in it then we have an exact dependency on the same version '== X.Y.Z', is that right?

Correct.

https://packaging.python.org/en/latest/specifications/version-specifiers/#compatible-release

If I read this right, "~= X.Y.Z.dev0" means

= X.Y.Z.dev0, X.Y.Z.*

But I'm a little fuzzy on whether it is " X.Y.Z.*" or "== X.Y.*" -- is "X.Y.Z.dev0" normalized as "X.Y.Z.0dev0" ?

So which would match any development package in the X.Y.Z series or subsequent X.Y.Z release?

The key bit is this (emphasis added):

If a pre-release, post-release or developmental release is named in a compatible release clause as V.N.suffix, then the suffix is ignored when determining the required prefix match

So when we write ~= X.Y.Z.dev0, that's equivalent to == X.Y.*, >= X.Y.Z.dev0. This isn't exactly what we want either to be sure, but it's much closer than the <= X.Y.Z we use in current main, and it would help to see what the actual failure modes are before making the requirements even more involved to guard against problems.

Anyway, this LGTM.

Thanks.