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

proposal: use setuptools/setuptools_scm for building #216

Merged
merged 2 commits into from
Oct 17, 2023

Conversation

dtrifiro
Copy link
Contributor

@dtrifiro dtrifiro commented Oct 2, 2023

This PR changes the build system from flit to setuptools+setuptools_scm. The main driver behind this PR is to simplify development by having setuptools_scm automatically set the module version at build/install time instead of having a fixed 0.0.1 version replaced by CI.

Another advantage of using setuptools_scm is to have proper versions reported when using editable installs, avoiding version requirements warnings/errors.

This also adds the possibility of using caikit_nlp.__version__ (or __version_tuple__) to check the module version at runtime

@dtrifiro
Copy link
Contributor Author

dtrifiro commented Oct 2, 2023

An example of what the version generated by setuptools-scm: 0.2.2.dev39+g8cfe9b8: it includes git tag, number of commits since last tag and the git ref for the build.

Signed-off-by: Daniele Trifirò <dtrifiro@redhat.com>
Signed-off-by: Daniele Trifirò <dtrifiro@redhat.com>
@dtrifiro
Copy link
Contributor Author

Hey @gabe-l-hart, do you have any bandwidth to have a look at this and/or discuss it?

Copy link
Collaborator

@gkumbhat gkumbhat left a comment

Choose a reason for hiding this comment

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

I like this change. @gabe-l-hart or @evaline-ju or @joerunde thoughts on this? Also do we want to include this for caikit as well?

Currently caikit-nlp cannot be published to pypi itself because of peft version, but something seems this would be useful for caikit and once we are able to push caikit-nlp to pypi.

@dtrifiro
Copy link
Contributor Author

I have a patch ready for caikit aswell if this is approved.

Copy link
Contributor

@joerunde joerunde left a comment

Choose a reason for hiding this comment

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

@dtrifiro yeah this looks better than what we whipped up when we pulled this all out into github. Thanks!

Can you go ahead and make the change to caikit as well?

@joerunde
Copy link
Contributor

Ah, I am not a code owner here so I'd actually need @gkumbhat to hit approve on this PR

Copy link
Contributor

@gabe-l-hart gabe-l-hart left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you!

@gabe-l-hart gabe-l-hart merged commit b750bd7 into caikit:main Oct 17, 2023
4 checks passed
@dtrifiro dtrifiro deleted the use-setuptools branch October 18, 2023 13:27
@dtrifiro
Copy link
Contributor Author

dtrifiro commented Oct 18, 2023

This PR had a small issue which was fixed in #239, see pypa/setuptools#3340 for more context.

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

4 participants