-
Notifications
You must be signed in to change notification settings - Fork 27
feat: --requirements-file option #748
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
Conversation
|
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
| python, | ||
| requirements_file="requirements.txt" if not force_generate else None, | ||
| python=python, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only used by vetiver-python package, which writes an application from scratch and deploys it using rsconnect. We can take for granted the requirements file is always "requirements.txt" here as it's driven by vetiver-python
| """ | ||
|
|
||
| if force_generate: | ||
| if requirements_file is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main change in terms of logic.
Externally we preserved the --force-generate command line option to retain backward compatibility, but internally the force_generate argument is now replaced by the None value passed as the the requirements_file
| dest="requirements_file", | ||
| default="requirements.txt", | ||
| help="Requirements file name (relative to the directory). Use 'none' to capture via pip freeze.", | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inspect_environment subprocess (the one that runs inside the application environment) now accepts a requirements file argument.
As the argument is passed via command line (thus strings) we use the "none" value to signal the None python value for the requirements_file argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this help is the right place or not, but we should be explicit that "requirements file" here could be both a requirements.txt or requirements.txt.lock file.
I suspect most of our users will read requirements file and think only of requirements.txt, and not think a lock file is even possible (let alone the thing this is possibly most useful for!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the internal inspect_environment.py script we run in a subprocess to inspect the application. It is not exposed to the user. I only changed it to use argparse for the purpose of making more clear (instead of previous positional arguments), but the user never sees this help nor the command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, maybe I missed this elsewhere, do we document this distinction / combination of concepts anywhere? Here isn't the right place, but I want to make sure we communicate to our users that --requirements-file requirements.txt.lock is a thing since IMO that's not actually obvious from the name of the argument alone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate?
I mean, if you explicitly use --requirements-file X then X could be named anything you want and doesn't even have to be a lockfile, it can be anything listing python dependencies.
So not sure I get what you mean by "we communicate to our users that --requirements-file requirements.txt.lock is a thing"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to improve a bit the "help=X" messages in #750
Feel free to comment on that PR with a better help message if you feel there is space for improvement
| click.secho( | ||
| " Warning: The requirements file '%s' is outside of the deployment directory.\n" % requirements_file, | ||
| fg="red", | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the user can now provide a requirements file, the used requirements file might be "../../../something.txt", we want to make sure the user has clear that's not meant to happen.
| # -*- coding: utf-8 -*- | ||
| import os | ||
| from datetime import date | ||
| from datetime import date # noqa: F401 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just to get rid of an already existing lint error
| if requirements_file and "jupyter" not in engines: | ||
| raise RSConnectException( | ||
| "--requirements-file is only supported for Quarto content using the Jupyter engine." | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that quarto has 3 possible values:
- jupyter → Python notebooks; needs a Python environment
- markdown → plain/markdown docs rendered by Quarto, no Python dependencies
- knitr → explicitly rejected by validate_quarto_engines
So we only support the requirements_file option when the engine is Jupyter
| :param: dirname Directory name | ||
| :param: force_generate Force the generation of an environment | ||
| :param: requirements_file The requirements file to read. If None, generate using pip freeze. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on if we could incorporate the spirit of #680 into this change too? pip freeze has never been satisfying to most of our users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean making the requirements file a required part of the project?
I think that's a bit out of scope here as it would involve changing the logic of --force-generate option.
I haven't touched the behavior of this function, the pip_freeze was already there, so it's not different from the past behavior. We can definitely enforce the requirements file in a separate PR as you were doing, happy to pick up that work in the context of that PR if you can't move it forward
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
happy to pick up that work in the context of that PR if you can't move it forward
Thank you, I would very much appreciate that!
toph-allen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering the internal logic of the changes in this PR, this generally looks fine to me, and if merging it is required for the demo you plan to give today, go ahead and merge.
I think I'm missing some context about the motivation of this change that I'd like to learn. Before this change, rsconnect-python would use a requirements.txt file if one existed in the current directory, right? So, is this change motivated by a desire to allow different names and directory structures? Or is there something else I'm unaware of?
| " Warning: Capturing the environment using 'pip freeze'.\n" | ||
| " Consider creating a requirements.txt file instead.", | ||
| " Consider creating a %s file instead." % requirements_file, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial impulse was that if a user explicitly passes in a requirements file path and we can't find a file there, we should emit an error, not a warning.
But I realize we don't have any way to know whether the path we have is one that was explicitly provided or the default from the user, so I guess I'm just noting my weird feeling here.
Intent
Allow users to specify a custom requirements file.
Closes #747
Type of Change
Approach
added a
--requirements-file/-roption to force a specific requirements file.Internally
force_generatehas been removed. Now a requirements file can be specified, when the requirements file isNoneit means one should be generated, thus replacing theforce_generateoption.To retain compatibility with current commands, the command line options have not been changed,
--force-generatestill exists. It just sets the requirements file toNoneAutomated Tests
test_requirements_overridetests for support in overriding requirements.Directions for Reviewers
Checklist