-
Notifications
You must be signed in to change notification settings - Fork 26
feat: --package-installer option #708
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
base: main
Are you sure you want to change the base?
Conversation
|
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
|
I apologize in advance for the driveby comment about naming, but: given that "Package Manager" is one of Posit's products, I worry that calling this argument I don't have an alternative to suggest, just wanted to flag this. Maybe this is fine? |
I think "--package-installer" could work since this is strictly about installation and not other package management operations. PyPI references "package installer":
The Python Packaging guide also generally qualifies "installation" The UV documentation is less precise:
|
in Python specific terms that would probably be the "Package Index": https://pip.pypa.io/en/latest/cli/pip_search/#cmdoption-i I guess that anyone using the PPM would be used to already specify that as the "index" both in |
I don't have a strong opinion, on this side there is a lot of confusion 🤷🏻
|
|
What about having I share Neal's concern that the unfortunate collision with the Posit product is likely to lead to confusion. If we had to we could recover from that being super explicit if someone puts something other than |
|
@jonkeane this has been open for a while. I'm not fond of |
Sure. I still truly believe that my original suggestion ( |
2748dae to
d93dc53
Compare
tdstein
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.
I think this looks good overall. The various possible configuration states is a bit confusing to me, but that may be unavoidable due to the server requirements.
I haven't run this code yet, so please let me know if you'd like me to sanity check that it works as expected.
| self._data._replace(**{name: value}) | ||
| self._data = self._data._replace(**{name: value}) |
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 was a bug?
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.
yep... somehow we never faced it, but the data was indeed never updated
| # Override the package manager name recorded by inspector | ||
| environment.package_manager = package_manager # type: ignore[attr-defined] | ||
| # Derive allow_uv from selection | ||
| environment.package_manager_allow_uv = True if package_manager == "uv" else False |
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.
Trying to confirm my understanding... Is package_manager_allow_uv the same as "set the package manager to uv"?
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.
python.package_manager.allow_uv in the manifest means: "Allow use of uv, if the server is configured to use it as the default package manager". When allow_uv=True if you configured python.package_manager.name=pip you allow the server to override your preference for "pip" if the server is configured to prefer "uv".
| python: typing.Optional[str] = None, | ||
| override_python_version: typing.Optional[str] = None, | ||
| app_file: typing.Optional[str] = None, | ||
| package_manager: typing.Optional[str] = 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.
Would a Literal be better here? It looks like click is enforcing that the value is uv or pip at the command line. So, internally we could just defer to the type system to make sure we aren't passing around other values.
| if package_manager not in ("pip", "uv"): | ||
| raise RSConnectException("Unsupported package manager: %s" % package_manager) |
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.
Changing package_manager to a Literal['uv', 'pip'] might remove the need for this check.
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.
Fair, I retained the pattern the code has of checking values at runtime,
I'll switch to a validated value. But in this case I'd consider StrEnum a much better choice as it's a domain specific value, it benefits from being reusable and it's runtime safe too.
| help='Force generating "requirements.txt", even if it already exists.', | ||
| ) | ||
| @click.option( | ||
| "--package-installer", |
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 think we discussed this elsewhere, but having "package installer" here and then "package manager" in the source code is a little confusing.
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.
package_manager is the actual name it has in the manifest.json, so the code related to generating the manifest uses that name. I agree that this is very confusing, but I feel it's even more surprising if you assign one value to the environment but then it writes a different one in the manifest.
| ) as bundle, tarfile.open(mode="r:gz", fileobj=bundle) as tar: | ||
| manifest = json.loads(tar.extractfile("manifest.json").read().decode("utf-8")) | ||
| assert manifest["python"]["package_manager"]["name"] == "uv" | ||
| assert manifest["python"]["package_manager"]["allow_uv"] is True |
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.
Again, I'm not sure why "allow_uv" is required here alongside "name" = "uv". It seems like the server should respond with an error if ["python"]["package_manager"]["name"] == "uv" but it doesn't support uv.
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.
It is not required as far as I remember, you are correct. I just preferred to avoid any gray areas and set both values to "use uv" and be consistent.
Intent
Allows users to pass a
--package-installer=pip|uvoption to force a specific package manager.By default it's up to the server (and it's backward compatible with server that didn't accept uv)
Closes #707
Type of Change
Approach
The
package_managersetting is now propagated in theEnvironmentobject and can be used by both deploy and write manifest processes.Automated Tests
Added tests for the 3 cases
=uv=pipDirections for Reviewers
If in doubt see https://docs.posit.co/connect/user/manifest/#python
Checklist