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

Build Wheel Action #105

Merged
merged 3 commits into from
Apr 13, 2024
Merged

Conversation

jbusche
Copy link
Collaborator

@jbusche jbusche commented Apr 2, 2024

Description of the change

This PR adds build and publish Github Actions that will allow extra testing of the build and wheel files with tox and then ultimately publish the library out on the production PyPI when we get the token figured out.

Related issue number

Closes issue 671 and external issue #57

How to verify the PR

From my github repo, I synced my branch with main and then am able to manually run the GHA successfully, giving it the input version, for example 1.0.dev3 in this run: https://github.com/jbusche/fms-hf-tuning/actions/runs/8547067789

I tried then installing from a new python env:

pip install fms-hf-tuning==1.0.dev3
pip install fms-hf-tuning[dev]==1.0.dev3
pip list |grep fms
fms-hf-tuning            1.0.dev3

Was the PR tested

Yes, by running the GHA in my fork. (The GHA won't run in the upstream repo until it's synced with main branch)

  • I have added >=1 unit test(s) for every new method I have added.
  • I have ensured all unit tests pass

@jbusche
Copy link
Collaborator Author

jbusche commented Apr 2, 2024

/retest

@jbusche jbusche force-pushed the jb-wheel-issue57 branch 2 times, most recently from 9847340 to e659341 Compare April 2, 2024 22:13
@jbusche jbusche force-pushed the jb-wheel-issue57 branch from 7a6bede to 6a83db9 Compare April 3, 2024 23:57
@jbusche jbusche marked this pull request as ready for review April 4, 2024 00:06
@jbusche
Copy link
Collaborator Author

jbusche commented Apr 4, 2024

OK this GHA works, building the wheel file and pushing up to PyPI.

Some questions remain:

  1. Is manually running the GHA and giving it the version number the right way to publish to PyPI or is it wanted to be automatic, for example when branch name = release or tag or something? If so, how do we want to increment the version number... be sure to update the toml file prior to pushing the release branch, or maybe from an individual version file?
  2. With the trusted upload to pypi, there is an "environment name" option. (see doc) It's recommended to set for extra security, but I don't know what the environment name would be or how to set that.
  3. In testing pushed 0.0.1 already, didn't realize it's not deletable. I need to research if, while not deletable if it's updatable with newer wheel file(s) on the same version.

@tedhtchang
Copy link
Collaborator

@z103cb Please advice.

Copy link

@z103cb z103cb left a comment

Choose a reason for hiding this comment

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

I think that this PR is ok. We should consider perhaps having this triggered from "Create release" page in GITHUB. An example from RH library I have collaborated on can be found here: https://github.com/opendatahub-io/caikit-nlp-client/blob/main/.github/workflows/release.yml

Comment on lines 43 to 46
- name: Replace release-version with provided input value
run: |
export RELEASE_VERSION=${{ github.event.inputs.release-version }}
sed -i "s/^version = \"[0-9.]*\"$/version = \"${RELEASE_VERSION}\"/g" pyproject.toml
Copy link

Choose a reason for hiding this comment

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

Perhaps we should commit this change to the file or conversely, do it in the way, it's done the here.

Copy link

Choose a reason for hiding this comment

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

i could not find version = is matched in pyproject.toml

Copy link
Collaborator

Choose a reason for hiding this comment

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

you may find version = in this pyproject.toml

Copy link
Collaborator

Choose a reason for hiding this comment

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

if we go with git release, then I think we won't need to do this sed here ? (since we would do it on tags)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK - working on this... thanks for the advice, reviews!

@z103cb
Copy link

z103cb commented Apr 4, 2024

OK this GHA works, building the wheel file and pushing up to PyPI.

Some questions remain:

  1. Is manually running the GHA and giving it the version number the right way to publish to PyPI or is it wanted to be automatic, for example when branch name = release or tag or something? If so, how do we want to increment the version number... be sure to update the toml file prior to pushing the release branch, or maybe from an individual version file?

I would prefer it be done automatically, when a new "GitHub release" is created. In the example I have provided, believe that the version is derived from the tag on the release. The relevant bits are in:

[build-system]
build-backend = "setuptools.build_meta"
requires = [
    "setuptools>=60",
    "setuptools-scm>=8.0",
]


[project]
name = "caikit_nlp_client"
dynamic = ["version"]
description = "caikit-nlp client"
license = { text = "Apache License 2.0" }
readme = "README.md"
classifiers=[
    "License :: OSI Approved :: Apache Software License",
    "Development Status :: 4 - Beta",
    "Programming Language :: Python :: 3",
    "Programming Language :: Python :: 3.9",
    "Programming Language :: Python :: 3.10",
    "Programming Language :: Python :: 3.11",
]
  1. With the trusted upload to pypi, there is an "environment name" option. (see doc) It's recommended to set for extra security, but I don't know what the environment name would be or how to set that.

I would ask the owners of the repo

  1. In testing pushed 0.0.1 already, didn't realize it's not deletable. I need to research if, while not deletable if it's updatable with newer wheel file(s) on the same version.

I would not worry too much to about it.

Copy link

@zdtsw zdtsw left a comment

Choose a reason for hiding this comment

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

some nitty gritty for the GH action part

.github/workflows/build-and-publish.yaml Outdated Show resolved Hide resolved
.github/workflows/build-and-publish.yaml Outdated Show resolved Hide resolved
Comment on lines 43 to 46
- name: Replace release-version with provided input value
run: |
export RELEASE_VERSION=${{ github.event.inputs.release-version }}
sed -i "s/^version = \"[0-9.]*\"$/version = \"${RELEASE_VERSION}\"/g" pyproject.toml
Copy link

Choose a reason for hiding this comment

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

i could not find version = is matched in pyproject.toml

@tedhtchang
Copy link
Collaborator

tedhtchang commented Apr 4, 2024

I would prefer it be done automatically, when a new "GitHub release" is created. In the example I have provided, believe that the version is derived from the tag on the release.

Update @jbusche I'v tested with those changes in a repo. I believe these are changes you need in
pyproject.toml and the build-and-push.yaml

[build-system]
requires = [
    "setuptools>=60",
    "setuptools-scm>=8.0"]

[project]
dynamic = ["version"]

[tool.setuptools_scm]
version_file = "tuning/_version.py"

This example would publish the package with 0.0.1rc-2 release in pypi
image

Update: The setuptools_scm actually writes to the _version.py file.
@z103cb This a good suggestion. Btw, the version comes from a file but I am not sure how the file got created.

.github/workflows/build-and-publish.yaml Outdated Show resolved Hide resolved
.github/workflows/build-and-publish.yaml Outdated Show resolved Hide resolved
Comment on lines 43 to 46
- name: Replace release-version with provided input value
run: |
export RELEASE_VERSION=${{ github.event.inputs.release-version }}
sed -i "s/^version = \"[0-9.]*\"$/version = \"${RELEASE_VERSION}\"/g" pyproject.toml
Copy link
Collaborator

Choose a reason for hiding this comment

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

if we go with git release, then I think we won't need to do this sed here ? (since we would do it on tags)

.github/workflows/build-and-publish.yaml Show resolved Hide resolved
@jbusche jbusche force-pushed the jb-wheel-issue57 branch from 2cd7ba4 to 5e6f1a5 Compare April 9, 2024 17:21
@jbusche
Copy link
Collaborator Author

jbusche commented Apr 9, 2024

@gkumbhat and @tedhtchang, thanks for the suggestions, I got it to work on release where it builds the wheel and publishes up to PiPY. I just tried with 0.0.1rc.5 from my repo (had to use my main bracn so it would work):
https://github.com/jbusche/fms-hf-tuning/actions/runs/8620146700/job/23626417173

pip install fms-hf-tuning==0.0.1rc.5
pip install fms-hf-tuning[dev]==0.0.1rc.5

I think the only thing perhaps left is that people didn't want it set to just python 3.9, so I added a matrix that added all three in a manor like I saw in caitkit-nlp However having 3.9, 3.10 and 3.11 ended up launching three identical workflows which isn't what I wanted, so I've made it just 3.11. Is that sufficient?

@jbusche jbusche force-pushed the jb-wheel-issue57 branch 2 times, most recently from d051df3 to 4ccd221 Compare April 10, 2024 20:19
tox.ini Show resolved Hide resolved
jbusche added 3 commits April 11, 2024 15:43
Signed-off-by: James Busche <jbusche@us.ibm.com>

release version in build workflow

Signed-off-by: James Busche <jbusche@us.ibm.com>

release version in build workflow

Signed-off-by: James Busche <jbusche@us.ibm.com>

fix build workflow for version

Signed-off-by: James Busche <jbusche@us.ibm.com>

space fix in build workflow for version

Signed-off-by: James Busche <jbusche@us.ibm.com>

Add publish to pypi

Signed-off-by: James Busche <jbusche@us.ibm.com>

condense publish to pypi

Signed-off-by: James Busche <jbusche@us.ibm.com>

cleanup build yaml

Signed-off-by: James Busche <jbusche@us.ibm.com>

Switch publish to release or tag

Signed-off-by: James Busche <jbusche@us.ibm.com>

adding py311 and fix dependencies

Signed-off-by: James Busche <jbusche@us.ibm.com>

remove version from setup.py and add debugging

Signed-off-by: James Busche <jbusche@us.ibm.com>

adding PiPY publish

Signed-off-by: James Busche <jbusche@us.ibm.com>
Signed-off-by: James Busche <jbusche@us.ibm.com>
Signed-off-by: James Busche <jbusche@us.ibm.com>
@jbusche
Copy link
Collaborator Author

jbusche commented Apr 11, 2024

PR 105 is looking good in testing from my main branch, testing creating a new release. It builds the wheel now without source (thanks for the tip @ted CHANG on how to only build the wheel file) and publishes up to PiPY. Can see that only the wheel file is published: https://pypi.org/project/fms-hf-tuning/0.0.1rc6/#files

Tried on a fresh python environment, and it's looking good:

pip install --upgrade pip
pip install fms-hf-tuning==0.0.1rc.6
pip install fms-hf-tuning[dev]==0.0.1rc.6

and the install looks good:

pip list |grep fms
fms-hf-tuning            0.0.1rc6

and I was successfully able to run the twitter_complaints test

100%|██████████████████████████████████████████████████████████████████████████████████████████| 3/3 [01:33<00:00, 31.08s/it]

Copy link
Collaborator

@tedhtchang tedhtchang 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.

setup.py Show resolved Hide resolved
@gkumbhat gkumbhat merged commit b747c5f into foundation-model-stack:main Apr 13, 2024
3 checks passed
@jbusche jbusche deleted the jb-wheel-issue57 branch April 15, 2024 17:15
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.

5 participants