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

Proposed fix for #474: enables parsing of requirements.txt during locking #476

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

stefansjs
Copy link

Description

This addresses #474, which fails to lock if environment.yml has -rrequirements.txt in the pip requirements. The main problem with the existing implementation is that it treats requirements lists as if they were a tool.poetry.dependencies block in a pyproject.toml. Poetry is great, and I see why you would want to vendor such a clean implementation, but poetry doesn't have any specification for external files. Further, any differences between calling pip install and poetry lock would eventually cause an issue.

My fix mimics conda's implementation which writes a temporary requirements.txt and invokes the command-line pip install command. https://github.com/conda/conda/blob/main/conda_env/installers/pip.py. Ideally this means there will be fewer bugs/differences between performing conda env create and conda lock.

Since pip doesn't provide an explicit library or API for parsing I do it by invoking pip's own parse_requirements function. I also based the return values strongly on the pyproject_toml.py implementation.

Testing and Feedback

I would love if somebody could suggest the right place to put unit tests.

@stefansjs stefansjs requested a review from a team as a code owner July 28, 2023 21:56
@netlify
Copy link

netlify bot commented Jul 28, 2023

Deploy Preview for conda-lock ready!

Name Link
🔨 Latest commit 41cae9a
🔍 Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/64c4399b190087000832f68e
😎 Deploy Preview https://deploy-preview-476--conda-lock.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

from conda_lock.src_parser.pyproject_toml import unpack_git_url


def parse_requirements_txt(file_path, category=None) -> Dependency:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the return type annotation, this seems to be Dependency generator rather than returning a Dependency.

Copy link
Contributor

@maresb maresb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really promising!!! Apologies for taking so long to provide feedback.

I'd recommend putting tests into tests/test_conda_lock.py.

I'm a bit uncomfortable tapping into pip._internal since that may be an unstable API. (Perhaps it's worth considering if it's feasible to vendor pip._internal.req.req_file.)

But overall I think this is a major step in the right direction and a very important feature. Thanks so much for the submission!

requirements.write("\n") # Trailing newline
requirements.file.close()

dependencies.extend(parse_requirements_txt(requirements.name, category))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering what will happen now with editable requirements.

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.

2 participants