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.

Sign up for GitHub

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

Jump to bottom

Read dependencies from requirement files for setup #26

Closed
wants to merge 2 commits into from
Closed

Read dependencies from requirement files for setup #26

wants to merge 2 commits into from

Conversation

ankitade
Copy link
Contributor

@ankitade ankitade commented Apr 13, 2022 •
edited
Loading

Summary:

  • setup.py now has install_requires which reads from requirements.txt
  • extra requires for dev specific dependencies coming from dev-requirements.txt (moved pytest to it)
  • pytorch and torch* dependencies still need to be installed manually since we depend on nightly builds and there seems to be no principled way of getting them in setup.py

Test plan:

  • In a fresh conda env, followed Readme and tried to import flava. Checked no dev dependencies were installed in the env
  • In another env, followed updated contribution guide and ran pytest. Verified mypy, pytest and precommit were installed

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 13, 2022
@ankitade ankitade force-pushed the build branch 2 times, most recently from 632fd0a to 1c901b8 Compare April 13, 2022 20:32
@ankitade ankitade marked this pull request as ready for review April 13, 2022 20:34
@facebook-github-bot
Copy link
Contributor

@ankitade has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

ebsmothers
ebsmothers reviewed Apr 14, 2022
View reviewed changes
dev-requirements.txt Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
@ebsmothers ebsmothers mentioned this pull request Apr 14, 2022
Closed
@facebook-github-bot
Copy link
Contributor

@ankitade has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

langong347
langong347 reviewed Apr 14, 2022
View reviewed changes
CONTRIBUTING.md Outdated
```

To perform type checking:
TorchMultimodal uses mypy for type checking.To perform type checking:
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a space here?
TorchMultimodal uses mypy for type checking.(space)To perform type checking:

README.md Outdated Show resolved Hide resolved
@langong347
Copy link
Contributor

We should consider adding version numbers in requirements file at least for packages where reproducible behaviors are important.

@ankitade
Copy link
Contributor Author

this is pretty much reproducing the manual readme right now so idk which ones we want to pin

@facebook-github-bot
Copy link
Contributor

@ankitade has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

langong347
langong347 reviewed Apr 15, 2022
View reviewed changes
CONTRIBUTING.md Outdated
@@ -33,7 +33,7 @@ outlined on that page and do not file a public issue.

Same as in [README](README.md) with the exception of:
```
python setup.py develop
pip install -e .
Copy link
Contributor

@langong347 langong347 Apr 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Does pip install -e .[mode] replace the two-step approach python setup.py [mode] + pip install -r [mode]-requirements? (I recall in your earlier version it was pip install -e .[dev]?)

Should we consider sticking with one flavor in our documentation (in README.md it still tells the users to do python setup.py install)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

strange, i meant to change it in requirements.txt and not in contributing but maybe i wasnt paying attention

@langong347
Copy link
Contributor

this is pretty much reproducing the manual readme right now so idk which ones we want to pin

Versions are added in requirements files. All packages should have versions to guarantee reproducibility across developers and (especially) in CI.

But I've also seen repos not being strict with versions (i.e., only package names in requirements). That should be fine until we become more sensitive to versions.

For now I'm just referring to the linter/formatter (see my comments earlier) as those are known issues.

@facebook-github-bot
Copy link
Contributor

@ankitade has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@ankitade has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@ankitade ankitade deleted the build branch April 20, 2022 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@langong347 langong347 langong347 left review comments

@ebsmothers ebsmothers ebsmothers approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ankitade @facebook-github-bot @langong347 @ebsmothers