Use Virtual Environment for Python Deps

[lindluni]

Fixes issues with python deps

Proposed Changes

1. ...
2. ...
3. ...

Readiness Checklist

Author/Contributor

• If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

• Label as breaking if this is a large fundamental change
• Label as either automation, bug, documentation, enhancement, infrastructure, or performance

[admiralAwkbar]

@lindluni took some liberties on your bash script :)

[admiralAwkbar]

@ferrarimarco this should help with #2224

[lindluni]

@admiralAwkbar I'll fix the issue later tonight. I need to build outside of virtualenv, otherwise it tries to link against that interpreter instead of the one installed in the container

[jalaziz]

I would recommend a separate requirements.txt for each dependency.

This will fail to upgrade certain dependencies that have fixed dependencies on each other (like snakefmt).

Also, black below is a regression compared to the current working version.

Dependabot works with directories, so you could have a python folder with a .txt for each tool, then do something like this in build-python-binaries.sh:

while read -r line; do
  # Install dependency
  pipx install "${line}"
done < ./python/*.txt

[jalaziz]

With this we can also remove the self-contained black binary install.

[ferrarimarco]

Maybe I'm missing the point, but how is this going to help? If dependencies are conflicting during updates, they're going to conflict during the installation as well, right?

[jalaziz]

Maybe I'm missing the point, but how is this going to help? If dependencies are conflicting during updates, they're going to conflict during the installation as well, right?

They won't because pipx installs each tool in its own virtualenv. Separation of the dependencies would allow dependabot to upgrade each tool independently. If they're in the same requirements.txt, then dependabot will try to ensure that the versions don't conflict and may hold back updates.

[lindluni]

Best I could figure pipx doesn't support requirement files, it runs purely npx. Switched to virtual environments instead. Can't say python is in any way shape or form my forte, but this works. Need to come back and revisit the size by pruning and reusing where we can, and need to update the README for adding new deps.

[lindluni]

I also need to add doc on updating the deps

[jalaziz]

Best I could figure pipx doesn't support requirement files...

It doesn't, but it does support the standard python requirements specifier. pipx should be sufficient because you should only need to install one tool at a time and not have to worry about the transitive dependencies.

One thing I noticed is that the there are multiple requirements listed per tool. Why is that? Instead of fixing transitive dependencies, it's probably better to just let pip / pipx / whatever figure it out.

Lastly, instead of having a directory per-tool with a requirements.txt, it would be a lot simpler to have one python-dependencies folder with a <tool>.txt for each tool. Dependabot doesn't seem care what the file is named as long as it ends with .txt. This would simplify the dependabot configuration considerably.

EDIT: Looking at the dependabot code again, it's not clear if separate requirements files in the same directory will result in dependabot trying to resolve each file separately or as one giant unit. I can probably come up with a quick test to verify and report back.

Can't say python is in any way shape or form my forte, but this works.

I'm happy to help if that would be useful.

Also, my apologies to Github staff if I'm stepping out of line with these comments, just happened to have looked into this already and I think it could be simpler.

[lindluni]

@jalaziz the issue with not fixing the transitive deps is you never know if a transitive dep has a vulnerability in it. Dependabot can only tell if the package listed has a vulnerability. So if I fail to list the transitive deps, we are at the mercy of the upstream project to patch the vulnerability, and we will never know if we are consuming vulnerable dependencies because the upstream failed to patch something.

It lets us make informed decisions about the dependencies we are consuming, to remove them, fork them, or to help patch an upstream project when we can.

And by no means are you out of line. We are a community and we are here to learn from each other.

[lindluni]

I'm also not saying my approach is best, good, or anything of the sort, I personally think the use of virtual environments is far too heavy, my first go around was to actually compile all the projects with pyinstaller, but ansible-core was a nightmare to do that with

[jalaziz]

@jalaziz the issue with not fixing the transitive deps is you never know if a transitive dep has a vulnerability in it. Dependabot can only tell if the package listed has a vulnerability. So if I fail to list the transitive deps, we are at the mercy of the upstream project to patch the vulnerability, and we will never know if we are consuming vulnerable dependencies because the upstream failed to patch something.

It lets us make informed decisions about the dependencies we are consuming, to remove them, fork them, or to help patch an upstream project when we can.

Yeah, that's totally fair. I was actually wondering if Dependabot would actually do the right thing after my comment and started questioning it myself. In that case, yeah, virtualenvs are probably a safe bet.

I'm also not saying my approach is best, good, or anything of the sort, I personally think the use of virtual environments is far too heavy, my first go around was to actually compile all the projects with pyinstaller, but ansible-core was a nightmare to do that with

Oh lord, I can imagine. That being said, virtualenvs may not be too bad. If setup correctly, virtual envs will share the system's python through symlinks. That would be roughly equivalent or better to what you'd be getting with PyInstaller (minus potential compression and other small optimizations).

[jalaziz]

Also, I think the Dependabot config will definitely need updating now as I don't believe it recurses into subdirectories (dependabot/dependabot-core#2178 and dependabot/dependabot-core#1015).

[lindluni]

Still need to update dependabot, and add the readme, but this is ready for everyone's thoughts

[admiralAwkbar]

dank fixes all around

[jalaziz]

Looks like the dependabot update was still missing from this PR. Should I open a new PR?

[admiralAwkbar]

@jalaziz PR's are always welcome :)
this open got out of hand lol