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

detectron2: require coco2017-minimal data #2022

Closed
wants to merge 2 commits into from

Conversation

davidberard98
Copy link
Contributor

It seems like detectron2 requires coco2017-minimal data. It gets installed with vision-maskrcnn, but if you want to install requirements only for one model we need the installer scripts for the detectron models to download coco2017-minimal.

It seems like detectron2 requires coco2017-minimal data. It gets installed with vision-maskrcnn, but if you want to install requirements only for one model we need the installer scripts for the detectron models to download coco2017-minimal.
@facebook-github-bot
Copy link
Contributor

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

Copy link
Contributor

@xuzhao9 xuzhao9 left a comment

Choose a reason for hiding this comment

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

LGTM with minor comment

@@ -55,6 +55,7 @@ def remove_tools_directory():

def install_detectron2(model_name, model_dir):
s3_utils.checkout_s3_data("INPUT_TARBALLS", "coco128.tar.gz", decompress=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could remove this? I remember detectron2 models only requires coco2017-minimal, not coco128 - I don't remember the details though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I grepped for coco128 and couldn't find anything other than instances of checkout_s3_data, so I think you're right - will do that.

@facebook-github-bot
Copy link
Contributor

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

@davidberard98 davidberard98 temporarily deployed to docker-s3-upload November 2, 2023 03:40 — with GitHub Actions Inactive
@davidberard98 davidberard98 temporarily deployed to docker-s3-upload November 2, 2023 03:40 — with GitHub Actions Inactive
@facebook-github-bot
Copy link
Contributor

@davidberard98 merged this pull request in 1e03c23.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants