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

Remove incorrect MAML implementation #1198

Closed
wants to merge 1 commit into from
Closed

Conversation

zou3519
Copy link
Contributor

@zou3519 zou3519 commented Sep 21, 2022

Fixes #328

Here's some context:

The last step to resolve this issue is to delete the incorrect MAML example, unless we have reasons to keep it around.

Fixes pytorch#328

Here's some context:
- We discovered that the implementation doesn't actually use maml
(dragen1860/MAML-Pytorch#59)
- We filed pytorch#328
- We added maml_omniglot pytorch#349
as the correct version of of the maml model.
- We didn't delete the maml model (because I was worried that it was
doing a "different" type of maml that I hadn't seen before that is still
valid).

The last step to resolve this issue is to delete the incorrect MAML
example, unless we have reasons to keep it around.
@xuzhao9
Copy link
Contributor

xuzhao9 commented Sep 21, 2022

It makes sense to me to remove maml and keep maml_omniglot as the correct impl.
Another stakeholder of the maml model is the torchdynamo team, @jansel are you okay with this PR?

@jansel
Copy link
Contributor

jansel commented Sep 21, 2022

maml is one of the hardest models for dynamo to handle because it involves a copy.deepcopy() and some dynamic autograd during inference.

I think it is pretty useful and would like to keep it, though perhaps more of a challenging test case and less as a benchmark.

@xuzhao9
Copy link
Contributor

xuzhao9 commented Sep 21, 2022

I see, let's keep this model then. I will close this PR, and create a separate PR to mark maml as correctness test only and will not be used for performance testing.

Thanks @jansel and @zou3519 !

@xuzhao9 xuzhao9 closed this Sep 21, 2022
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.

Replace MAML with an actually correct implementation
4 participants