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 the use of generator in dispatcher #1264

Merged
merged 31 commits into from
Feb 26, 2020
Merged

Conversation

vdutor
Copy link
Contributor

@vdutor vdutor commented Feb 18, 2020

GPflow makes use of a multipledispatch Dispatcher that internally uses a generator. However, according to TensorFlow Capabilities and Limitations generators are not supported by AutoGraph and probably will never be. Thus, compiling code that passed though the dispatcher led to the following warnings:

WARNING:tensorflow:Entity <bound method Dispatcher.dispatch_iter of <dispatched sample_conditional>> appears to be a generator function. It will not be converted by AutoGraph.
WARNING: Entity <bound method Dispatcher.dispatch_iter of <dispatched sample_conditional>> appears to be a generator function. It will not be converted by AutoGraph.
WARNING:tensorflow:Entity <bound method Dispatcher.dispatch_iter of <dispatched conditional>> appears to be a generator function. It will not be converted by AutoGraph.
WARNING: Entity <bound method Dispatcher.dispatch_iter of <dispatched conditional>> appears to be a generator function. It will not be converted by AutoGraph.

This PR still uses the same dispatcher, but overwrites the problematic method that uses python generators and replaces it by a simple list.

NOTE

As of this PR we do not need to write autograph=False in tf.function anymore, and all the code inside dispatching gets compiled the same way as everything else :) !

doc/source/notebooks/advanced/mcmc.pct.py Outdated Show resolved Hide resolved
doc/source/notebooks/advanced/mcmc.pct.py Outdated Show resolved Hide resolved
doc/source/notebooks/advanced/multioutput.pct.py Outdated Show resolved Hide resolved
doc/source/notebooks/tailor/external-mean-function.pct.py Outdated Show resolved Hide resolved
doc/source/notebooks/tailor/gp_nn.pct.py Outdated Show resolved Hide resolved
doc/source/notebooks/tailor/gp_nn.pct.py Outdated Show resolved Hide resolved
gpflow/utilities/multipledispatch.py Show resolved Hide resolved
gpflow/utilities/multipledispatch.py Show resolved Hide resolved
tests/test_coregion.py Outdated Show resolved Hide resolved
tests/test_coregion.py Outdated Show resolved Hide resolved
tests/test_coregion.py Outdated Show resolved Hide resolved
tests/test_dynamic_shapes.py Outdated Show resolved Hide resolved
tests/test_dynamic_shapes.py Outdated Show resolved Hide resolved
tests/test_gplvm.py Outdated Show resolved Hide resolved
tests/test_method_equivalence.py Outdated Show resolved Hide resolved
tests/test_method_equivalence.py Outdated Show resolved Hide resolved
tests/test_method_equivalence.py Outdated Show resolved Hide resolved
tests/test_multioutput.py Outdated Show resolved Hide resolved
tests/test_multioutput.py Outdated Show resolved Hide resolved
tests/test_natural_gradient.py Outdated Show resolved Hide resolved
tests/test_natural_gradient.py Outdated Show resolved Hide resolved
tests/test_natural_gradient.py Outdated Show resolved Hide resolved
tests/test_optimizers.py Outdated Show resolved Hide resolved
tests/test_multioutput.py Outdated Show resolved Hide resolved
tests/test_multioutput.py Outdated Show resolved Hide resolved
tests/test_multioutput.py Outdated Show resolved Hide resolved
tests/test_multioutput.py Outdated Show resolved Hide resolved
tests/test_multioutput.py Outdated Show resolved Hide resolved
tests/test_multioutput.py Outdated Show resolved Hide resolved
Co-Authored-By: Artem Artemev <art.art.v@gmail.com>
Co-Authored-By: st-- <st--@users.noreply.github.com>
@codecov

This comment has been minimized.

@vdutor
Copy link
Contributor Author

vdutor commented Feb 19, 2020

@st-- , @awav : addressed all comments. Ready to be merged now.

Copy link
Member

@awav awav left a comment

Choose a reason for hiding this comment

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

@vdutor +2,068 −47,924. I wish it were actual removing :)

Copy link
Contributor

@cdmatters cdmatters left a comment

Choose a reason for hiding this comment

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

2 comments:

  1. I worry a bit aboutthe naming of the original package being multipledispatch (class Dispatcher), and our custom file being also called multipledispatch (class Dispatcher). its only a fat finger that changes import .multipledispatch to import multipledispatch! Why doent we change the name of our utils file to gpflow_multipledispatch or custom_multipledispatch
  2. Secondly we should have some tests for this diff! A simple test is to create a dispatched function using the two dispatchers, checking the results are the same, but that one generates warnings when wrapped in tf.function!

@st-- st-- changed the base branch from develop to st/make_notebooks_deterministic February 19, 2020 15:53
@awav
Copy link
Member

awav commented Feb 19, 2020

I upvote @condnsdmatters suggestion on testing the warning. Although I don't think we need to test multipledispatch, and we definitely can use with warnings.catch_warnings(record=True) as w: to check that new Dispatch with tf.function gives no warnings.

@vdutor vdutor linked an issue Feb 25, 2020 that may be closed by this pull request
@st-- st-- changed the base branch from st/make_notebooks_deterministic to develop February 25, 2020 17:42
Co-Authored-By: Artem Artemev <art.art.v@gmail.com>
@vdutor
Copy link
Contributor Author

vdutor commented Feb 25, 2020

Ready for merge once tests pass.

Copy link
Member

@awav awav left a comment

Choose a reason for hiding this comment

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

Looks good

@st--
Copy link
Member

st-- commented Feb 25, 2020

Will merge this once #1265 is in develop

@awav
Copy link
Member

awav commented Feb 25, 2020

@st--, it is a bad practice to work on the branch from another author. Especially, when author gives green light for merging ;) I was about to hit the button.

@st--
Copy link
Member

st-- commented Feb 25, 2020

@awav yeah but this branch was off the other one and I wanted to merge them separately

@vdutor vdutor merged commit f8ee093 into develop Feb 26, 2020
@vdutor vdutor deleted the vincent/multipledispatch branch February 26, 2020 09:52
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.

Multiple dispatch
4 participants