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

[examples] Extract MultiDataLoader/Module to examples/common/data #93

Closed
wants to merge 5 commits into from
Closed

[examples] Extract MultiDataLoader/Module to examples/common/data #93

wants to merge 5 commits into from

Conversation

IvanKobzarev
Copy link
Contributor

@IvanKobzarev IvanKobzarev commented Jun 15, 2022 •
edited
Loading

Stack from ghstack (oldest at bottom):

We want to extract common functionality (MultiDataModule, MultiDataLoader) to be shared between flava and other examples.

Introducing examples/common/data/multidata.py. To be able to use it from examples/flava we need to be in one python package - examples =>

Renaming imports with prefix flava..

Training flava after this change will be python -m flava.train config=... to run it within examples package

Differential Revision: D37188387

@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 Jun 15, 2022
This was referenced Jun 15, 2022
Closed
Closed
@codecov-commenter
Copy link

codecov-commenter commented Jun 15, 2022 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (gh/ivankobzarev/5/base@849b649). Click here to learn what that means.
The diff coverage is n/a.

@@                    Coverage Diff                    @@
##             gh/ivankobzarev/5/base      #93   +/-   ##
=========================================================
  Coverage                          ?   88.29%           
=========================================================
  Files                             ?       34           
  Lines                             ?     1837           
  Branches                          ?        0           
=========================================================
  Hits                              ?     1622           
  Misses                            ?      215           
  Partials                          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 849b649...9d5afbb. Read the comment docs.

@IvanKobzarev IvanKobzarev requested a review from ankitade June 15, 2022 21:23
@IvanKobzarev
Copy link
Contributor Author

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

ebsmothers
ebsmothers reviewed Jun 16, 2022
View reviewed changes
Copy link
Contributor

@ebsmothers ebsmothers left a comment

Choose a reason for hiding this comment

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

Overall these changes look good. One question though: why move the requirements file up a level? I see the value in making a lot of these data utils common (which necessitates at least some dependencies in examples/), but I think the idea is for each example to have its own distinct set of requirements. For instance, someone importing a generic data util probably does not want to take a dep on DALL-E in general.

@IvanKobzarev
Copy link
Contributor Author

Overall these changes look good. One question though: why move the requirements file up a level? I see the value in making a lot of these data utils common (which necessitates at least some dependencies in examples/), but I think the idea is for each example to have its own distinct set of requirements. For instance, someone importing a generic data util probably does not want to take a dep on DALL-E in general.

Thanks, I agree, it will be better to have separate requirements files per example model.

@IvanKobzarev
Update on "[examples] Extract MultiDataLoader/Module to examples/comm…
9d5afbb 
…on/data"


We want to extract common functionality (MultiDataModule, MultiDataLoader) to be shared between flava and other examples.

Introducing `examples/common/data/multidata.py`. To be able to use it from `examples/flava` we need to be in one python package - `examples` =>

Renaming imports with prefix `flava.`.

Training flava after this change will be `python -m flava.train config=...` to run it within `examples` package

Differential Revision: [D37188387](https://our.internmc.facebook.com/intern/diff/D37188387)

[ghstack-poisoned]
@IvanKobzarev
Copy link
Contributor Author

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

@IvanKobzarev IvanKobzarev requested a review from ebsmothers June 16, 2022 23:16
@IvanKobzarev
Update on "[examples] Extract MultiDataLoader/Module to examples/comm…
04c34fc 
…on/data"


We want to extract common functionality (MultiDataModule, MultiDataLoader) to be shared between flava and other examples.

Introducing `examples/common/data/multidata.py`. To be able to use it from `examples/flava` we need to be in one python package - `examples` =>

Renaming imports with prefix `flava.`.

Training flava after this change will be `python -m flava.train config=...` to run it within `examples` package

Differential Revision: [D37188387](https://our.internmc.facebook.com/intern/diff/D37188387)

[ghstack-poisoned]
@IvanKobzarev
Copy link
Contributor Author

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

ebsmothers
ebsmothers approved these changes Jun 17, 2022
View reviewed changes
Copy link
Contributor

@ebsmothers ebsmothers left a comment

Choose a reason for hiding this comment

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

Other than the one minor comment, this looks good to go.

examples/flava/README.md Outdated
@@ -21,7 +21,7 @@ First, clone the repo, install `multimodal` and then install requirements for th
git clone https://github.com/facebookresearch/multimodal.git
cd multimodal
pip install -e .
cd examples/flava
cd examples
pip install -r requirements.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pip install -r requirements.txt
pip install -r flava/requirements.txt

@IvanKobzarev
Update on "[examples] Extract MultiDataLoader/Module to examples/comm…
51bd3aa 
…on/data"


We want to extract common functionality (MultiDataModule, MultiDataLoader) to be shared between flava and other examples.

Introducing `examples/common/data/multidata.py`. To be able to use it from `examples/flava` we need to be in one python package - `examples` =>

Renaming imports with prefix `flava.`.

Training flava after this change will be `python -m flava.train config=...` to run it within `examples` package

Differential Revision: [D37188387](https://our.internmc.facebook.com/intern/diff/D37188387)

[ghstack-poisoned]
@IvanKobzarev
Copy link
Contributor Author

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

@facebook-github-bot facebook-github-bot deleted the gh/ivankobzarev/5/head branch June 21, 2022 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebsmothers ebsmothers ebsmothers approved these changes

@ankitade ankitade Awaiting requested review from ankitade

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
@IvanKobzarev @codecov-commenter @ebsmothers @facebook-github-bot