Conversation
|
jonkeane
force-pushed
the
require_requirements
branch
from
61d8d73 to
cbd2cf4
Compare
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
|
I agree that making requirements.txt generation explicit instead of implicit is the correct direction.
|
amol-
force-pushed
the
require_requirements
branch
from
cbd2cf4 to
4d6b2ea
Compare
|
@jonkeane mind reviewing the changes when you have a chance? |
jonkeane
left a comment
jonkeane
left a comment
There was a problem hiding this comment.
I can't literally approve because I opened the PR, but this should be treated as an approval (thanks, github).
We don't have to here, but I do think we should still consider the following separately
Possibly replacing it with https://github.com/bndr/pipreqs (or we could pull in the go code that the publisher uses).
| if requirements_file is None: | ||
| result = pip_freeze() | ||
| elif os.path.basename(requirements_file) == "uv.lock": | ||
| result = uv_export(dirname, requirements_file) | ||
| else: | ||
| result = output_file(dirname, requirements_file, "pip") or pip_freeze() | ||
| result = output_file(dirname, requirements_file, "pip") | ||
| if result is None: | ||
| raise EnvironmentException( | ||
| "The requirements file '%s' was not found in '%s'. " | ||
| "Please create it or use --force-generate to use pip freeze." % (requirements_file, dirname) | ||
| ) |
There was a problem hiding this comment.
This might be due to being away from this block for so long, but it took me a second to realize/remember that requirements_file here is just the name of the file, so that first if will only be false when it's literally passed as None (i.e. when someone passes --force-generate), and not when that file doesn't exist (that's lower down in the else).
I think this is fine, and maybe this comment is enough details in the git record, but it did take me a second to figure out how the code change here was even accomplishing what we wanted it to!
There was a problem hiding this comment.
The logic is more or less:
- requirements_file was requested -> use it and error if it doesn't exist
- requirements_file was not requested -> generate a new one (ie `--force-generate`)
Happy to improve the comment in any way you feel it might make this more obvious.
Maybe I can just add an explicit comment within the if block stating that --force-generate causes requirements_file to be None
There was a problem hiding this comment.
block stating that --force-generate causes requirements_file to be None
Yeah, that would have helped me when I was reading
tdstein
left a comment
tdstein
left a comment
There was a problem hiding this comment.
Is there a simple way to make this use uv pip freeze if it's available?
I guess so, is there any specific improvement you think that would provide? Environments without pip are very uncommon as in most cases Also the inspect process explicitly runs in a target python environment, so there isn't the need for |
Co-authored-by: Taylor Steinberg <taylor@steinberg.xyz>
|
3 participants
Intent
Require a requirements file by default, do not provide one. This is partially an idea to see how others feel about it. I would argue that we actually should remove the pip freeze fallback entirely, honestly. Possibly replacing it with https://github.com/bndr/pipreqs (or we could pull in the go code that the publisher uses).
Resolves #538
Type of Change
Approach
Mostly Claude, actually
Automated Tests
Changed
Checklist
Will do at some point: