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

Automate pypi deployment #919

Merged
merged 2 commits into from
Jun 23, 2024
Merged

Conversation

hmoazam
Copy link
Collaborator

@hmoazam hmoazam commented Apr 28, 2024

WIP for automatically publishing package to pypi and creating github release. Testpypi automation in place for package at: https://test.pypi.org/project/dspy-ai-test/

@hmoazam
Copy link
Collaborator Author

hmoazam commented May 6, 2024

Github action for automating deployments and versioning of the dspy-ai package on PyPI.

Checklist for use included at docs/docs/release-checklist.md

Overview of the workflow:

  1. Maintainer of the repo pushes a tag following semver versioning for the new release.
  2. This triggers the github action which extracts the tag (the version)
  3. Builds and publishes a release on test-pypi
  4. Uses the test-pypi release to run build_utils/tests/intro.py with the new release as an integration test. Note intro.py is a copy of the intro notebook.
  5. Assuming the test runs successfully, it pushes a release to pypi. If not, the user can delete the tag, make the fixes and then push the tag again. Versioning for multiple releases to test-pypi with the same tag version is taken care of by the workflow by appending a pre-release identifier, so the user only needs to consider the version for pypi.
  6. (Currently manual) the user creates a release and includes release notes, as described in docs/docs/release-checklist.md

@hmoazam hmoazam marked this pull request as ready for review May 6, 2024 18:26
@hmoazam hmoazam requested a review from ammirsm May 6, 2024 23:18
Copy link
Collaborator

@ammirsm ammirsm left a comment

Choose a reason for hiding this comment

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

Great job! Thank you for your time and contribution!
Overall, this looks good to me, but here are a few suggestions:

  1. It would be helpful to have a document explaining how we are building this. Including this document in the PR should be sufficient. While I have a general understanding of how it works currently, a short document connecting the dots would make it much easier to understand.
  2. I've added a few comments which may help in cleaning up, but they aren't necessary.
  3. Instead of copying the intro.py into the file, it might be better to move it to pytest and run it from there.

@@ -0,0 +1,223 @@
###
# Copy of intro notebook for the sake of testing new versions of the package.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would say let's move this to a pytest and run that test case instead of running the actual intro.py file. what do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes that's a good call, I'll do that. Shall I put it under tests/examples?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would say that would be a great place to put or we can move all of these tests to a new folder name tests/integration and keep them there, this way we can distinct it later to run them or not to run them where ever we want.

I think it is good to separate these integration and maybe later we want to run them in their own CI step.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can help with setting up this one for you, it will be a little bit tricky, I will push it in another branch and add it to CI for you in couple days.

Copy link
Collaborator

Choose a reason for hiding this comment

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

#1048

I added my initial commits here.

setup.py Show resolved Hide resolved
python-version: "3.9"
- name: Install package from TestPyPI
run: |
for i in {1..5}; do
Copy link
Collaborator

Choose a reason for hiding this comment

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

for keeping this file clean do you wanna move this to the .sh file?

@hmoazam
Copy link
Collaborator Author

hmoazam commented May 18, 2024

Great job! Thank you for your time and contribution! Overall, this looks good to me, but here are a few suggestions:

  1. It would be helpful to have a document explaining how we are building this. Including this document in the PR should be sufficient. While I have a general understanding of how it works currently, a short document connecting the dots would make it much easier to understand.

-> Shall I expand what I've mentioned here or is that enough detail? I also included a release-checklist.md file which contains instructions for use and prereqs.

  1. I've added a few comments which may help in cleaning up, but they aren't necessary.
  2. Instead of copying the intro.py into the file, it might be better to move it to pytest and run it from there.

@ammirsm
Copy link
Collaborator

ammirsm commented May 20, 2024

Great job! Thank you for your time and contribution! Overall, this looks good to me, but here are a few suggestions:

  1. It would be helpful to have a document explaining how we are building this. Including this document in the PR should be sufficient. While I have a general understanding of how it works currently, a short document connecting the dots would make it much easier to understand.

-> Shall I expand what I've mentioned here or is that enough detail? I also included a release-checklist.md file which contains instructions for use and prereqs.

  1. I've added a few comments which may help in cleaning up, but they aren't necessary.
  2. Instead of copying the intro.py into the file, it might be better to move it to pytest and run it from there.

@hmoazam I think that's a good draft but i would say

  1. let's keep it somewhere in our repo as a document not a comment
  2. let's add a little bit more detail like which part of the CI is doing what

All in all, great job!

Let's tweak it a little bit and merge it as soon as we can.

@hmoazam
Copy link
Collaborator Author

hmoazam commented Jun 22, 2024

This PR depends on #1192 and #1048.

Once #1048 is merged in we need to:

  • Update the test-intro-script task to point to the pytest at tests_integration/test_intro.py instead of build_utils/tests/intro.py
  • Delete build_utils/tests/intro.py

@hmoazam hmoazam force-pushed the automate-pypi-deployment branch 2 times, most recently from d01378d to 5af03b7 Compare June 23, 2024 16:30
Copy link
Collaborator

@ammirsm ammirsm left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the hard work here @hmoazam 🚀🥳

@okhat okhat merged commit 01e5066 into stanfordnlp:main Jun 23, 2024
5 checks passed
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.

None yet

3 participants