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

Read Package Version Better #1415

Merged
merged 7 commits into from
Jul 31, 2024
Merged

Conversation

eitanturok
Copy link
Contributor

@eitanturok eitanturok commented Jul 31, 2024

In composer, streaming, and MegaBlocks, we read the package version in the setup.py file from the _version.py file. However, in llm-foundry we are missing a _version.py file and instead read the version from the llmfoundry/__init__.py file. This PR changes this.

To ensure consistency with our other repos, I put the package version in the _version.py file. Having a _version.py file better follows good design principles as it is better to separate the version out into its own file.

@eitanturok eitanturok requested review from a team as code owners July 31, 2024 16:02
@eitanturok eitanturok requested a review from dakinggg July 31, 2024 16:07
Copy link
Contributor

@snarayan21 snarayan21 left a comment

Choose a reason for hiding this comment

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

minor change, other than that lgtm

setup.py Outdated Show resolved Hide resolved
@eitanturok eitanturok requested a review from snarayan21 July 31, 2024 16:29
Copy link
Contributor

@snarayan21 snarayan21 left a comment

Choose a reason for hiding this comment

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

lgtm

llmfoundry/_version.py Outdated Show resolved Hide resolved
@eitanturok eitanturok requested a review from dakinggg July 31, 2024 18:28
@eitanturok
Copy link
Contributor Author

eitanturok commented Jul 31, 2024

Weird, I initially imported __version__ into the __init__.py file. Looking at the GA logs here, turns out the pycln hook was removing __version__, probably because it was an unused import.

However, when I ran the pre-commit hooks locally, isort removed __version__ because it was an unused import.

To fix this, I added __version__ to the __all__ so it will hopefully think the import is being used.

Not sure what is going on.

@eitanturok eitanturok merged commit c9d09ce into mosaicml:main Jul 31, 2024
9 checks passed
@eitanturok eitanturok deleted the eitan-version branch July 31, 2024 20:40
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.

4 participants