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

Cleanup MegaBlocks Installation #126

Closed
wants to merge 26 commits into from
Closed

Cleanup MegaBlocks Installation #126

wants to merge 26 commits into from

Conversation

eitanturok
Copy link
Contributor

@eitanturok eitanturok commented Jul 24, 2024

Update 2024-07-25:

  1. We will use stk for now and not my fork. We'll take care of this in a separate PR.
  2. We will update the repo author and email in a separate PR.
  3. I added a _version.py for versioning and we dynamically read the version in setup.py.

This PR cleans up the MegaBlocks installation process. Specifically, it

  1. Adds a pyproject.toml file
  2. Updates the README.md installation instructions
  3. Ignores more files in .gitignore
  4. Adds missing packages to setup.py, i.e. torch and numpy
  5. Adds missing packages for testing to megablocks[dev], i.e. coverage, pytest, pytest_codeblocks, pytest-cov, pre-commit.
  6. Gracefully handles missing torch while running setup.py
  7. Fix style in setup.py: replade ' with " and add license header
  8. Improve long_description by parsing README.md better (taken from composer)
  9. MegaBlocks depends on stk. stk's setup.py unnecessarily imports torch which causes issues with installing stk before we have torch installed. We now install my fork of stk that fixes this issue while waiting for @tgale96 to merge my PR fixing this in stk.

@eitanturok eitanturok marked this pull request as ready for review July 24, 2024 13:11
@eitanturok eitanturok mentioned this pull request Jul 24, 2024
Copy link
Collaborator

@b-chu b-chu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@eitanturok
Copy link
Contributor Author

eitanturok commented Jul 24, 2024

I addressed all of your comments @b-chu. I based my pyproject.toml and setup.py based on Composer copying the features and style while paying careful attention to only copy code that is necessary for MegaBlocks.

My fork of stk enables us to set up github actions for MegaBlocks, something we do not need to worry about right now. So I'm going to just use the regular stk version so we can merge this.

LMK if that sounds good to you.

@eitanturok eitanturok requested a review from b-chu July 24, 2024 21:09
.gitignore Outdated Show resolved Hide resolved
megablocks/__init__.py Show resolved Hide resolved
setup.py Show resolved Hide resolved
@eitanturok
Copy link
Contributor Author

Are we ready to merge? Any other reviewers we should notify?

@eitanturok eitanturok requested a review from b-chu July 25, 2024 16:31
@eitanturok eitanturok assigned dakinggg and unassigned dakinggg Jul 25, 2024
@eitanturok eitanturok requested review from dakinggg and removed request for b-chu July 25, 2024 20:16
@karan6181
Copy link

Tagging @mvpatel2000 for the review before we merge this PR.

@eitanturok eitanturok requested review from mihir-db and removed request for mihir-db July 25, 2024 20:20
Copy link
Collaborator

@dakinggg dakinggg left a comment

Choose a reason for hiding this comment

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

LGTM, but wanted to note that since this PR includes a pyproject.toml, I think it enables build isolation which may make the package more difficult to install. Since the torch dep is specified in pyproject.toml it should be ok, but upgrading to latest torch versions will need to be done with care.

README.md Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@eitanturok eitanturok marked this pull request as draft July 26, 2024 20:48
@eitanturok
Copy link
Contributor Author

eitanturok commented Jul 26, 2024

Currently using custom forks of packages to resolve installation issues:

Blocked until these are merged. @dakinggg

@eitanturok
Copy link
Contributor Author

Closing this PR since #127 changed a lot of things.

@eitanturok eitanturok closed this Jul 31, 2024
@eitanturok eitanturok mentioned this pull request Jul 31, 2024
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