-
Notifications
You must be signed in to change notification settings - Fork 79
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
Fix nightly-release workflow and tweak Poetry package naming #208
Conversation
…rather than setup.py which was removed, tweak package naming
currentDate=$(date +%Y%m%d%H%M%S) | ||
versionNumber=$(cat $versionFile) | ||
devVersion="${versionNumber}.dev${currentDate}" | ||
echo "$devVersion" > $versionFile |
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 all looks reasonable to me. I was trying to think of some edge cases where it would be possible to perform a partial or incomplete version bump, but theoretically if any of these subcommand were to fail, the step itself would fail and then the GitHub Action job would fail.
@@ -22,8 +22,11 @@ exclude = ''' | |||
''' | |||
|
|||
[tool.poetry] | |||
name = "pinecone" | |||
name = "pinecone-client" |
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.
Interesting. I went back and forth on this name value in my original Poetry PR from pinecone-client
back to pinecone
to get everything working. However your explanation in the PR description makes sense to me.
packages = [ | ||
{ include="pinecone", from="." }, | ||
] |
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.
Read up on this and it makes sense to me.
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.
Thanks for tackling this! Your changes make sense to me. Might want to pull in a second set of eyes all the same.
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.
These changes look reasonable to me. The nightly release is a safe place to experiment so I'm in favor of merging and iterating more if we find this change to pyproject.toml causes an issue.
## Problem `nightly-release` workflow is still failing after the previous "fix": #208. This time it's due to `make package` failing on `poetry build`, because `poetry` wasn't setup properly in the workflow. https://github.com/pinecone-io/pinecone-python-client/actions/runs/6399744230/job/17372332268 > Run make package poetry build make: poetry: No such file or directory make: *** [Makefile:22: package] Error [12](https://github.com/pinecone-io/pinecone-python-client/actions/runs/6399744230/job/17372332268#step:9:13)7 Error: Process completed with exit code 2. While fixing this I noticed we're also not installing Poetry in the `publish-to-pypi` workflow, so calling `poetry version` and `make` commands there would fail. We need to make sure we're installing Poetry properly before using `make` in our workflows, as they call Poetry under the hood. ## Solution - Add a new `setup-poetry` action that encapsulates the install + dependencies bits. I noticed we were doing this explicitly in several workflows and thought it called for an action. - Fix `nightly-release` worfklow: call the new action before calling `make package`. - Fix `release-to-pypi` worfklow: call the new action before calling `poetry version`, and `make` commands for publishing. - Clean up / use the action in `testing` worfklow. ## Type of Change - [X] Infrastructure change (CI configs, etc) ## Test Plan Validate PR workflows pass as expected. Merge and check `nightly-release` to make sure we're in good shape.
Problem
After the poetry migration in #193 the
PyPI Release: Nightly (pinecone-client-nightly)
has been failing: https://github.com/pinecone-io/pinecone-python-client/actions/runs/6374232183/job/17298658421The reason is we removed the
setup.py
file as a part of the migration andnightly-release.yaml
used to adjust the module name viased -i 's/pinecone-client/pinecone-client-nightly/g' setup.py
. To achieve something similar now that we've changed our build / packaging process to use Poetry, we need to look at updating thepyproject.toml
file.While looking at correcting this, I realized the resulting build artifacts from
make package
(which now callspoetry build
) now have different package names which may cause issues with PyPI:pinecone-client-2.2.4.tar.gz
pinecone-2.2.4.tar.gz
pinecone_client-2.2.4-py3-none-any.whl
pinecone-2.2.4-py3-none-any.whl
Solution
nightly-release.yaml
to callsed -i ...
on thepyproject.toml
file instead ofsetup.py
. This should swap outname: pinecone-client
forname: pinecone-client-nightly
under[tools.poetry]
which should fix up the package naming.packages
section topyproject.toml
to define the top-level pinecone package entrypoint. Poetry uses thename
field by default unless this is specified which is why we were ending up with artifacts calledpinecone-...
rather thanpinecone-client-...
.Note:
Poetry swaps the hyphen in
pinecone-client
for an underscore in the resulting build output artifacts.New packages:
pinecone_client-2.2.4.tar.gz
pinecone_client-2.2.4-py3-none-any.whl
The
.whl
naming is equivalent, and thetar.gz
has an underscore rather than a hyphen. I think ultimately this shouldn't matter given the guidance aroundname
here:https://packaging.python.org/en/latest/guides/distributing-packages-using-setuptools/#name
Type of Change
Test Plan
Describe specific steps for validating this change.