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 prototype datasets CI and related things #6944

Merged
merged 7 commits into from
Nov 11, 2022

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Nov 10, 2022

As sad as it is, this PR does three things:

  1. Remove the datasets v2 tests from CI
  2. Remove the decoding transforms since encoded data can only come from datasets v2
  3. Move the features.Encoded* into the torchvision.prototype.datasets namespace, since they are only used there.

cc @seemethere @bjuncek

@@ -152,15 +152,6 @@ commands:
args: --no-build-isolation <<# parameters.editable >> --editable <</ parameters.editable >> .
descr: Install torchvision <<# parameters.editable >> in editable mode <</ parameters.editable >>

install_prototype_dependencies:
Copy link
Member

@NicolasHug NicolasHug Nov 10, 2022

Choose a reason for hiding this comment

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

thanks Philip, Do you mind just leaving that one last time, to make sure CI is still green with the new changes (and then we can remove it in this PR)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You commented on the CircleCi config, but these test continue to run. We only removed the dependencies there. You meant that I should run the datasets tests again, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've temporarily reinstated the prototype datasets CI in 215fb18. LMK if that is not what you wanted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

CI is red, but for an unrelated failure on the transforms tests. Datasets tests are green.

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

The changes on the Transforms LGTM, thanks.

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks Philip

I see some prototype failures in the [REVERT ME] diff that aren't present in the latest one. I assume they were fixed in-between in #6934 ? If that's the case, LGTM. Thanks!

@pmeier pmeier merged commit ad2ecea into pytorch:main Nov 11, 2022
@pmeier pmeier deleted the rip-proto-datasets branch November 11, 2022 09:38
facebook-github-bot pushed a commit that referenced this pull request Nov 14, 2022
Summary:
* remove prototype datasets from CI

* move encoded features to prototype datasets namespace

* remove decoding transforms

* [REVERT ME] reinstate prototype datasets CI

* Revert "[REVERT ME] reinstate prototype datasets CI"

This reverts commit 215fb18.

Reviewed By: NicolasHug

Differential Revision: D41265183

fbshipit-source-id: 0ad525314ac72aa784cd329fa17e9776b42bba46
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.

4 participants