-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Porting MetaLearning to PyTorch #3653
Conversation
|
||
# Optimize it. | ||
|
||
learner = SineLearner() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be in an explicit test_ method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
deepchem/metalearning/torch_maml.py
Outdated
task. | ||
Example | ||
-------- | ||
import deepchem as dc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't formatted correctly as an example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected
deepchem/metalearning/torch_maml.py
Outdated
|
||
def __init__( | ||
self, | ||
learner, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add type annotations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
deepchem/metalearning/torch_maml.py
Outdated
|
||
Example | ||
-------- | ||
class SineLearner(dc.metalearning.TorchMetaLearner): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected
d38e587
to
dc9ae93
Compare
.. autoclass:: deepchem.metalearning.TorchMetaLearner | ||
:members: | ||
|
||
Torch MAML |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, space here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected
has_pytorch = False | ||
|
||
|
||
class SineLearner(TorchMetaLearner): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add this to a try
block, TorchMetaLearner may not be imported always (might be a cause for the failures of the Jax Tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added but now "SineLearner" is not defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can try running without the try
block locally to see what might be causing the error, that way you would be able to check if there is some dependency that is causing the error on CI or if there is some bug in the code which is causing the code to skip the definition
has_pytorch = False | ||
|
||
|
||
class SineLearner(TorchMetaLearner): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add this to a try
block, TorchMetaLearner may not be imported always (might be a cause for the failures of the Jax Tests
73fcf27
to
e5ffa0b
Compare
@pytest.mark.torch | ||
def test_maml_pytorch(self): | ||
|
||
class SineLearner(dc.metalearning.TorchMetaLearner): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can try dc.metalearning.torch_maml.TorchMetaLearner
if the tests fail again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I tried this. torch_maml is not recognized now in CI.
deepchem/metalearning/__init__.py
Outdated
try: | ||
from deepchem.metalearning.maml import MAML, MetaLearner | ||
except ModuleNotFoundError: | ||
pass | ||
from deepchem.metalearning.torch_maml import TorchMAML, TorchMetaLearner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should just be MAML/MetaLearner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dc.metalearning.MAML
for TF and then dc.metalearning.torch_mamll.MAML
for Torch. Then in a later version we swap the default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay
|
||
try: | ||
import torch | ||
import torch.nn.functional as F |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From deepechem.metalearning.torch_maml import MetaLearner
import torch | ||
import torch.nn.functional as F | ||
|
||
class SineLearner(dc.metalearning.TorchMetaLearner): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try the same thing as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's break this into 2 PRs, one for MetaLearning and one for MAML
has_pytorch = False | ||
|
||
|
||
class TestMAML(unittest.TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need the torch marker to prevent this beining.
@pytest.mark.torch | ||
def test_maml_pytorch(self): | ||
|
||
class SineLearner(MetaLearner): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe move this out of the test and into the try/catch block above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jax tests are passing now sir.
import pytest | ||
from flaky import flaky | ||
import unittest | ||
from deepchem.metalearning.torch_maml import MetaLearner, MAML |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add this line within the try-except block as it is also indirectly importing torch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Jax tests are now passing.
import deepchem as dc | ||
import numpy as np | ||
import pytest | ||
from deepchem.metalearning.torch_maml import MetaLearner, MAML |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add this line within the try-except block as it is also indirectly importing torch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
Jax tests are passing now.
deepchem/metalearning/torch_maml.py
Outdated
has_pytorch = False | ||
|
||
|
||
class MetaLearner(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NimishaDey Is this a duplicate of the existing MetaLearner class? We shouldn't reimplement the abstract superclass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sir I have added the 'parameter' method to the existing methods of MetaLearner class. It returns an iterator over the learner parameters. This is needed to construct PyTorch optimizer.
|
||
MetaLearner | ||
---------------- | ||
This is the abstract superclass for metalearning algorithms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NimishaDey This is a duplicate of the documentation above. We shouldn't reimplement the abstract superclass. Instead let's fix it as needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
.. autoclass:: deepchem.metalearning.torch_maml.MetaLearner | ||
:members: | ||
|
||
MAML |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mention this is the torch implementation and the earlier implementation is the TensorFlow implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add the new MAML abstract class to the shared documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NimishaDey Getting closer but we still need one more round of fixes. We shouldn't reimplement the abstract superclass in particular
deepchem/metalearning/__init__.py
Outdated
raise NotImplementedError("Subclasses must implement this") | ||
|
||
def parameters(self): | ||
"""This method is specific to PyTorch implementation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Abstract classes should not refer to implementation (so no pytorch mention). Talk about the general idea). Also formatting for this string is off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected
deepchem/metalearning/__init__.py
Outdated
def parameters(self): | ||
"""This method is specific to PyTorch implementation. | ||
Returns an iterator over the MetaLearner parameters.""" | ||
print("Subclasses must implement this for PyTorch implementation.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raise not implemented error not a print
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected
has_pytorch = False | ||
|
||
|
||
class MAML(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also duplicated with Tensorflow. We should have a shared abstract class instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documenting offline discussion, there is currently no abstract MAML superclass so we made a parallel torch implementation to existing TF. We may want to factor out an abstract class later
@@ -1,12 +1,16 @@ | |||
Metalearning | |||
============ | |||
One of the hardest challenges in scientific machine learning is lack of access of sufficient data. Sometimes experiments are slow and expensive and there's no easy way to gain access to more data. What do you do then? | |||
One of the hardest challenges in scientific machine learning is lack of access of sufficient data. Sometimes experiments are slow and expensive and there's no easy way to gain access to more data. What do you do then? | |||
|
|||
This module contains a collection of techniques for doing low data | |||
learning. "Metalearning" traditionally refers to techniques for | |||
"learning to learn" but here we take it to mean any technique which | |||
proves effective for learning with low amounts of data. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have the abstract classes listed here.
f'Skipped loading some PyTorch models, missing a dependency. {e}') | ||
|
||
|
||
class MetaLearner(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will cause a circular import issue. Move this into its own file metalearner.py and importt it before the other files up top
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a circular import because we are not inheriting from it.
@@ -10,60 +10,6 @@ | |||
from deepchem.models.optimizers import Adam, GradientDescent | |||
|
|||
|
|||
class MetaLearner(object): | |||
"""Model and data to which the MAML algorithm can be applied. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break the tensorflow code to break because the import will be gone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there is no circular import Tensorflow tests are running and I have the screenshot of it below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready to merge once @NimishaDey does final check of tests
Both PyTorch and Tensorflow tests are running. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We may still need to do some refinement but we can do in future PR
Description
Fix #(issue)
Type of change
Please check the option that is related to your PR.
Checklist
yapf -i <modified file>
and check no errors (yapf version must be 0.32.0)mypy -p deepchem
and check no errorsflake8 <modified file> --count
and check no errorspython -m doctest <modified file>
and check no errors