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

Tensorflow independent environment #2567

Merged
merged 7 commits into from
Jun 28, 2021
Merged

Conversation

VIGNESHinZONE
Copy link
Contributor

Fix #2561

  • I have added try & catch based on a few sub-packages in deepchem like metalearning, keras_models, rl because these sub-packages are tensorflow dependent.
  • Didn't have a lot of problem in trans subpackage because the TensorFlow dependency was local.

cc: @peastman @rbharath

#2563 requires a tensorflow free environment for smoothly creating a deepchem-[torch] environment.

@VIGNESHinZONE
Copy link
Contributor Author

In transformer subpackage, only IRVTransformer & ANITransformer have tensorflow depedency.

class IRVTransformer(Transformer):

class ANITransformer(Transformer):

Can we move these features into a seperate file called tensorflow_transformers and eventually do this in __init__.py

try:
  from deepchem.trans.transformers import IRVTransformer
  from deepchem.trans.transformers import ANITransformer
except:
  pass

@peastman
Copy link
Contributor

It looks to me like they're just using TensorFlow for a bunch of standard math operations that could just as easily be done with Numpy. I suggest we convert them.

I notice though that there don't seem to be any tests for ANITransformer, and also that it's written in TensorFlow 1.x style. I wouldn't be surprised if it doesn't actually work, but no one noticed due to the lack of tests and because no one is using it.

@codecov-commenter
Copy link

codecov-commenter commented Jun 24, 2021

Codecov Report

Merging #2567 (21dde75) into master (ab4bdff) will increase coverage by 0.31%.
The diff coverage is 78.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2567      +/-   ##
==========================================
+ Coverage   84.85%   85.17%   +0.31%     
==========================================
  Files         314      314              
  Lines       28084    27972     -112     
==========================================
- Hits        23831    23824       -7     
+ Misses       4253     4148     -105     
Impacted Files Coverage Δ
deepchem/trans/__init__.py 100.00% <ø> (ø)
deepchem/metalearning/__init__.py 50.00% <50.00%> (-50.00%) ⬇️
deepchem/rl/__init__.py 63.93% <60.00%> (-1.59%) ⬇️
deepchem/models/__init__.py 85.36% <84.00%> (-8.92%) ⬇️
deepchem/trans/transformers.py 86.09% <100.00%> (+13.31%) ⬆️

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 ab4bdff...21dde75. Read the comment docs.

@VIGNESHinZONE
Copy link
Contributor Author

So should i put ANITransformer in a separate file like mentioned above or remove it completely?
removed tf dependency from IRVtransformer

@rbharath
Copy link
Member

@VIGNESHinZONE I think for ANITransformer, it's been known to be broken for some time so let's just comment it out. We'll have to note it in #2553.

@rbharath
Copy link
Member

I think this PR will also need to be rebased since some merge conflicts have crept it

@VIGNESHinZONE
Copy link
Contributor Author

@rbharath I have made the changes.

Copy link
Member

@rbharath rbharath left a comment

Choose a reason for hiding this comment

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

Ok, I think this is ready to merge in. I'm seeing only 1 test failure on a test unrelated to this PR.

@peastman Do you have any remaining review comments?

@peastman
Copy link
Contributor

Looks good to me.

@rbharath rbharath merged commit d62afe4 into deepchem:master Jun 28, 2021
@VIGNESHinZONE VIGNESHinZONE deleted the tf-free branch July 10, 2021 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make TensorFlow optional
4 participants