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

Pin dependency versions to match current poetry.lock #3104

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ttys0dev
Copy link
Contributor

@ttys0dev ttys0dev commented Sep 5, 2023

If we want to be manually managing package version updates for all packages in pyproject.toml we should just pin the exact package versions, this simplifies updating of transient dependencies in out poetry.lock in which we don't manually manage updates for.

This pins all versions in out pyproject.toml based on our current poetry.lock file.

@mlissner
Copy link
Member

mlissner commented Sep 5, 2023

Not sure how this will change things in practice, but it feels like a break in semantics. The old way indicated that we see point releases as OK, but not minor releases, so if somebody wants to do a point release upgrade, great. This way indicates that we trust nothing and that any version bump needs careful review. I think we're closer to the former than the latter (though I usually review all release notes anyway because I don't trust anything that's not super popular).

So that's one count against this. What does this make easier for you though? Maybe that'd weigh in its favor?

@ttys0dev
Copy link
Contributor Author

ttys0dev commented Sep 5, 2023

This way indicates that we trust nothing and that any version bump needs careful review.

I mean, this seems to be how we've been handling updates so far, at least for packages in pyproject.toml I think.

What does this make easier for you though?

This makes it possible to do a poetry lock instead of poetry lock --no-update without desynchronizing the versions in pyproject.toml from poetry.lock. This simplifies updating transient dependencies significantly.

@mlissner
Copy link
Member

mlissner commented Sep 5, 2023

I thought we landed updates to all the transient ones the other day such that poetry lock should be OK now (and it might grab some new transient junk, but that's OK)?

@ttys0dev
Copy link
Contributor Author

ttys0dev commented Sep 5, 2023

I thought we landed updates to all the transient ones the other day such that poetry lock should be OK now (and it might grab some new transient junk, but that's OK)?

The issue is that poetry lock will update all non-transient caret dependencies as well. Strict pinning like this prevents that.

@mlissner
Copy link
Member

mlissner commented Sep 5, 2023

OK, I gotcha. Yeah, I think poetry is a bit weird that way, but what I do is poetry add some-dep@latest, and that does the update that I want. I think it's better to keep the semantics the way we like them and to deal with poetry's strange ergonomics than it is to change the semantics to make the poetry commands easier.

@ttys0dev
Copy link
Contributor Author

ttys0dev commented Sep 5, 2023

what I do is poetry add some-dep@latest, and that does the update that I want

This doesn't really work correctly for updating transient dependencies because it will also add the transient to pyproject.toml.

@ttys0dev
Copy link
Contributor Author

ttys0dev commented Sep 5, 2023

FYI you can use poetry show -o to show packages that have updates.

@mlissner
Copy link
Member

mlissner commented Sep 5, 2023

Are you sure? That would surprise me because doing poetry add isn't supposed to add anything except add the one specific dep?

@ttys0dev
Copy link
Contributor Author

ttys0dev commented Sep 6, 2023

That would surprise me because doing poetry add isn't supposed to add anything except add the one specific dep?

But we're not actually trying to add a transient dependency to pyproject.toml, we only say want to update it in poetry.lock.

@cweider
Copy link
Collaborator

cweider commented Sep 6, 2023

Terminology-wise, we’re talking about transitive dependencies, not transient dependencies, right? (did autocorrect send things down the confusing path?)

Could it be that the command that would do the update that you’re seeking is not lock nor add, but update:

% poetry update $TRANSITIVE

I know that it worked for updating the transitive dependency on cryptography in #3044.

@ttys0dev
Copy link
Contributor Author

ttys0dev commented Sep 6, 2023

Terminology-wise, we’re talking about transitive dependencies, not transient dependencies, right? (did autocorrect send things down the confusing path?)

Oh, must have mixed that up.

Could it be that the command that would do the update that you’re seeking is not lock nor add, but update:

Yeah, I guess that does technically work I suppose, although it's less convenient as you can't bulk update transitive dependencies AFAIU that way.

@mlissner
Copy link
Member

mlissner commented Sep 6, 2023

It feels like this PR can be closed, and maybe we just need a wiki page about updating dependencies?

@cweider
Copy link
Collaborator

cweider commented Sep 6, 2023

A recipe in the documentation with xargs and cut could be just the thing.

@mlissner
Copy link
Member

mlissner commented Sep 6, 2023

So, summarizing, because I'm bad at this:

  • Update a core dependency — poetry add xyz@latest
  • Update a transitive dependency — poetry update transitive
  • Update all transitive dependencies — Something involving cut and xargs? I searched for the "right" way to do this, but didn't find anything.

@cweider
Copy link
Collaborator

cweider commented Sep 6, 2023

This should do the trick:

% poetry show -o | cut -d\  -f1 | xargs poetry update -- 

Doing it locally in cl-django highlights some developer experience setbacks I introduced with #3107 and #3110. To update dependencies in cl-django with docker exec, you need to specify --user root and use $POETRY_HOME/bin/poetry, which is awfully inconvenient. I’ll be sending over a change to make poetry available in $PATH and look into setting up a proper virtual environment for Python.

@ttys0dev
Copy link
Contributor Author

ttys0dev commented Sep 6, 2023

This should do the trick:

% poetry show -o | cut -d\  -f1 | xargs poetry update -- 

This appears to be functionally equivalent to poetry lock as it updates non-transitive dependencies as well.

@ttys0dev
Copy link
Contributor Author

ttys0dev commented Sep 6, 2023

Hmm, so we would need to filter this list of packages poetry show --top-level | cut -d\ -f1 out of poetry show -o or something like that I think to only get the transitives.

@cweider
Copy link
Collaborator

cweider commented Sep 7, 2023

How’s this:

% join -o 1.1 -v 1 <(poetry show -o) <(poetry show --top-level)

@mlissner
Copy link
Member

mlissner commented Sep 9, 2023

Well, I played with that a bit, but:

www-data@7cd487166b8c:/opt/courtlistener$ join -o 1.1 -v 1 <(/opt/poetry/bin/poetry show -o) <(/opt/poetry/bin/poetry show --top-level)
Skipping virtualenv creation, as specified in config file.
Skipping virtualenv creation, as specified in config file.

("Connection broken: PermissionError(13, 'Permission denied')", PermissionError(13, 'Permission denied'))

Then:

www-data@7cd487166b8c:/opt/courtlistener$ sudo join -o 1.1 -v 1 <(/opt/poetry/bin/poetry show -o) <(/opt/poetry/bin/poetry show --top-level)
bash: sudo: command not found
www-data@7cd487166b8c:/opt/courtlistener$ Skipping virtualenv creation, as specified in config file.
Skipping virtualenv creation, as specified in config file.

("Connection broken: PermissionError(13, 'Permission denied')", PermissionError(13, 'Permission denied'))

Which...I think I could figure out, but I think it's related to the www-data changes you made, Chad, and now I'm wondering how to get around them. I've gotten worse at this stuff!

@cweider
Copy link
Collaborator

cweider commented Sep 10, 2023

Oh, yea, it’s a www-data thing. Adding the --user root option will take care of it.

The alternative that would make this unnecessary is a virtual environment accessible to www-data. Exploring that has been a disappointment: poetry commands must be run from /opt/pysetup; the default working directory, /opt/courtlistener, won’t work. On balance, it seems like --user root is the better option.

@ttys0dev
Copy link
Contributor Author

How’s this:

% join -o 1.1 -v 1 <(poetry show -o) <(poetry show --top-level)

Well it works when running outside of docker for me at least.

So I guess full command for that would be:

join -o 1.1 -v 1 <(poetry show -o) <(poetry show --top-level) | xargs poetry update --

@mlissner
Copy link
Member

OK, yep, this is working, once Chad pointed out that --user root needs to be used to get into docker.

join -o 1.1 -v 1 <(/opt/poetry/bin/poetry show -o) <(/opt/poetry/bin/poetry show --top-level)

I've updated the wiki with this info: https://github.com/freelawproject/courtlistener/wiki/Managing-dependencies

Anything else to do here?

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

Successfully merging this pull request may close these issues.

None yet

3 participants