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

[sharktank] Revise setup.py file #346

Merged
merged 3 commits into from
Oct 30, 2024
Merged

[sharktank] Revise setup.py file #346

merged 3 commits into from
Oct 30, 2024

Conversation

marbre
Copy link
Collaborator

@marbre marbre commented Oct 28, 2024

  • Move version_info.json to sharktank subdir
  • Pulls in the local requirements file for sharktank

@marbre marbre requested a review from ScottTodd October 28, 2024 10:40
VERSION_INFO_FILE = REPO_DIR / "version_info.json"
VERSION_INFO_FILE = THIS_DIR / "version_info.json"
Copy link
Member

Choose a reason for hiding this comment

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

shortfin is using this too:
https://github.com/nod-ai/SHARK-Platform/blob/ffb146bdb3442c1d6d3a7417f334181a5834d117/shortfin/setup.py#L143-L144

Should we have a separate source of version information for each package, or require that they share a version?

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, let's have a separate one please to be able to raise the patch level individually. But I am fine with having it more similar to shortfin. Will adopt accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

I'm confused by git/github here. Am I missing something about your updates since my comment?

shortfin/setup.py should also be updated and a second version_info.json should be added

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems more likely that I am confused. I assumed that SOURCE_DIR you pointed to would refer to shortfin/ and that the way to get the version in shortfin works (as is). If this is the case shortfin is indeed missing a version_info.json. If that's not the case my assumption that SOURCE_DIR refers to shortfin is plainly wrong. Let me take a second look...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As the CI miserably fails with SOURCE_DIR not defined I definitely need to take a second look. Sorry for any noise.

Copy link
Member

Choose a reason for hiding this comment

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

Heh. We're both confused 😅. At least things should be more clear once we have the two files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, iterated further on the setup.py files and altered the PR / title description accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

Oooh wait, I'd forgotten about https://github.com/nod-ai/SHARK-Platform/blob/main/.github/workflows/build_packages.yml. That may need some updates too, but we'll be reworking it with new version information anyways.

What that does now is it generates a single version_info.json file for shortfin to use then extracts that file into the shortfin/ directory.

What we should end up with is a local file that determines the major.minor.patch then the workflows will append a suffix into that version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, let me roll back the changes applied to shortfin. We can harmonize these in a follow up.

@marbre marbre requested a review from ScottTodd October 28, 2024 16:42
@marbre marbre changed the title [sharktank] Move version_info.json to subdir Revise setup.py files Oct 28, 2024
Furthermore, unify the way the path to the version file is determined
(now similar to shortfin).
Instead of pulling in the upper-level requirements file, only pull in
a smaller subset defined in the recently added requirements file.
@marbre marbre changed the title Revise setup.py files [sharktank] Revise setup.py file Oct 29, 2024
@marbre
Copy link
Collaborator Author

marbre commented Oct 29, 2024

This should be in a state that it can be landed as is. Happy if you could stamp @ScottTodd.

@marbre marbre merged commit c6c7321 into nod-ai:main Oct 30, 2024
4 checks passed
@marbre marbre deleted the version_info branch October 30, 2024 12:57
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.

2 participants