Enhancement/1089 build method clarity #1140

kieranricardo · 2020-03-24T01:09:20Z

This PR cleans up the API for creating models. It makes several core changes:

On APPNP, Attri2Vec, GCN, GAT, GraphSAGE, HinSAGE, PPNP, the node_model and link_model methods are now private. Migration:
- model.node_model(): use model.in_out_tensors() (if the model was created with a node generator) or model.in_out_tensors(multiplicity=1) (otherwise)
- model.link_model(): use model.in_out_tensors() (if the model was created with a link generator) or model.in_out_tensors(multiplicity=1) (otherwise)
build is renamed to in_out_tensors for all model classes. Migration: replace model.build() with model.in_out_tensors()

In support of this, it makes some other changes:

tests and demos now call in_out_tensors instead of node_model/link_model/build/default_model
adds a multiplicity keyword arg into in_out_tensors for models with a multiplicity attribute

See #1089 and #1116

codeclimate · 2020-03-24T01:09:59Z

Code Climate has analyzed commit 0e8865d and detected 5 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Security	5

View more on Code Climate.

kjun9

Hmm when this was refactoring was proposed, I was under the impression that build (now in_out_tensors) would be calling node_model and link_model internally (i.e. the two methods are the helper methods used by in_out_tensors after checking multiplicity). But since this doesn't seem to be the case, is there any point in having those two private methods at all? Who are they intended to be used by?

huonw · 2020-03-24T05:13:14Z

This is going to be a bit annoying, but I feel like we really should have deprecations here. This is such a massive change that I imagine almost all users of StellarGraph will hit it (only those doing pure Node2vec or similar won't, I think), and as it stands, I think they'll hit an error like AttributeError: 'GCN' object has no attribute 'build' which is... rather unhelpful. It's such a fundamental change that we should potentially retain these deprecations for more than one release cycle. I can see a few approaches to implementing this (whichever you choose, you'll need to verify that it works as expected, though):

manual build methods

def build(self, *args, **kwargs):
    """Deprecated: use :meth:`in_out_tensors`."""
    warnings.warn("The 'build' method is deprecated, use 'in_out_tensors' instead.", DeprecationWarning, stacklevel=2)
    return self.in_out_tensors(*args, **kwargs)

defining a single function and setting it in each class

We could do the previous one somewhat magically, by defining a single instance of it (e.g. in stellargraph/layer/misc.py):

def deprecated_model_build(object, *args, **kwargs):
    """Deprecated: use :meth:`in_out_tensors`."""
    warnings.warn("The 'build' method is deprecated, use 'in_out_tensors' instead.", DeprecationWarning, stacklevel=2)
    return object.in_out_tensors(*args, **kwargs)

and then each model includes something like:

from .misc import deprecated_model_build
class GCN:
    ...
    build = deprecated_model_build

defining a base class for models with a `build` method

class Model:
    def build(self, *args, **kwargs):
        """Deprecated: use :meth:`in_out_tensors`."""
        warnings.warn("The 'build' method is deprecated, use 'in_out_tensors' instead.", DeprecationWarning, stacklevel=2)
        return self.in_out_tensors(*args, **kwargs)

...

class GCN(Model):
    ...

class GraphSAGE(Model):
    ...

We should consider doing the same for the node_model and link_model methods, although that's less major of a change. That is, retaining the public node_model and link_model methods with their current behaviour, but have them emit a DeprecationWarning.

Also, I feel like this really didn't need to have quite this many changes in it? The overall change isn't that large, but conceptually it's quite a lot, e.g. the last two changes seem pretty orthogonal to this change? (As in, things that don't depend on the name of the build method)

deprecates default_model

adds a multiplicity keyword arg into in_out_tensors for models with a multiplicity attribute

kieranricardo · 2020-03-24T05:14:59Z

Who are they intended to be used by?

I think having a seperate _node_model and _link_model now mostly just reduces code duplication because most _link_model functions end up calling _node_model twice

kieranricardo · 2020-03-24T05:36:48Z

@huonw

adds a multiplicity keyword arg into in_out_tensors for models with a multiplicity attribute

Some models like attri2vec could be created with a generator of any multiplicity, and then used to create either a link or a node model with link_model and node_model regardless of the generator.multiplicity. When I made node_model and link_model private, the attri2vec build function had no multiplicity kwarg so this got rid of the functionality of using any attri2vec instance to create a link and node model. I added the multiplicity kwarg for attri2vec and a couple of other models to keep this functionality.

deprecates default_model

I felt this fit into unifying the API to only use in_out_tensors, but I think you have a good point that this may be better suited to another PR.

kieranricardo · 2020-03-24T05:45:39Z

@huonw good point about the deprecations, I think I'll go for the deprecated_model_build approach because it seems easier to extend to node_model and link_model than your 3rd suggestion and less effort than your first suggestion 👍

…batch tests

huonw · 2020-03-24T22:05:37Z

I felt this fit into unifying the API to only use in_out_tensors, but I think you have a good point that this may be better suited to another PR.

Yeah, it's easy to do as a separate PR; in fact, I'd already opened #1145 doing it before I'd seen this PR.

huonw · 2020-03-24T22:16:36Z

stellargraph/layer/graph_attention.py

@@ -843,6 +843,10 @@ def __init__(
                )
            )

+        self.node_model = deprecated_model_function(self._node_model, "node_model")


Sorry to re-review before being suggested, but my original suggestion was careful to create these statically in each class. To be more specific, this would be like

class GAT: ... def _node_model(self): ... node_model = deprecated_model_function(_node_model, "node_model") link_model = deprecated_model_function(_link_model, "link_model") build = deprecated_model_function(in_out_tensors, "build")

This approach should mean they're still shown in the documentation, unlike dynamically assigning them when building each instance. After doing this, and moving the doc string from deprecated_model_function to _function_wrapper, the docs look like:

Compared to the current approach of this PR of https://stellargraph--1140.org.readthedocs.build/en/1140/api.html#stellargraph.layer.graph_attention.GAT.in_out_tensors which just shows in_out_tensors.

Exact diff of changes

diff --git a/stellargraph/layer/graph_attention.py b/stellargraph/layer/graph_attention.py index 65f5b8e0..a0a8df35 100644 --- a/stellargraph/layer/graph_attention.py +++ b/stellargraph/layer/graph_attention.py @@ -843,10 +843,6 @@ class GAT: ) ) - self.node_model = deprecated_model_function(self._node_model, "node_model") - self.link_model = deprecated_model_function(self._link_model, "link_model") - self.build = deprecated_model_function(self.in_out_tensors, "build") - def __call__(self, inputs): """ Apply a stack of GAT layers to the input x_inp @@ -964,3 +960,7 @@ class GAT: "Node model requested but a generator not supporting nodes was supplied." ) return self.in_out_tensors(multiplicity=1) + + node_model = deprecated_model_function(_node_model, "node_model") + link_model = deprecated_model_function(_link_model, "link_model") + build = deprecated_model_function(in_out_tensors, "build") diff --git a/stellargraph/layer/misc.py b/stellargraph/layer/misc.py index f97de342..6512632f 100644 --- a/stellargraph/layer/misc.py +++ b/stellargraph/layer/misc.py @@ -97,9 +97,9 @@ class SqueezedSparseConversion(Layer): def deprecated_model_function(function, old_name): - """Deprecated: use :meth:`in_out_tensors`.""" def _function_wrapper(*args, **kwargs): + """Deprecated: use :meth:`in_out_tensors`.""" warnings.warn( f"The '{old_name}' method is deprecated, use 'in_out_tensors' instead.",

nice good point!

@huonw my current approach isn't triggering deprecation warnings, can you see anything wrong?

my current approach isn't triggering deprecation warnings, can you see anything wrong?

What do you mean by this specifically? How are you testing?

Per #1117, the stacklevel needs to be set correctly (although it looks like it is), and, even with this, they'll only show up in Jupyter, or in script run with Python 3.7 not Python 3.6.

I'm testing some models in test_deprecated_model_function in test_misc.py, and I've also tried using the depracted functions in jupyter notebooks. The functions work but I'm not getting a warning

i'm getting jupyter warnings now nevermind, must just be my test

The `default_model` methods are really truly deprecated in favour of `build` (at the moment: #1140, #1089).

# Conflicts: # demos/node-classification/gcn/gcn-cora-node-classification-example.ipynb # stellargraph/layer/deep_graph_infomax.py # tests/layer/test_deep_graph_infomax.py # tests/layer/test_knowledge_graph.py

huonw

Nice work on this. There's a few little things, but otherwise I think it's good.

I checked the docs, and they seem to be as we expect, e.g.

huonw · 2020-03-25T02:15:08Z

tests/layer/test_misc.py

+        pass
+
+
+def test_deprecated_model_functions():


huonw · 2020-03-25T02:17:14Z

stellargraph/layer/rgcn.py

            return self._node_model()

        else:
            raise NotImplementedError(
                "Currently only node prediction if supported for RGCN."
            )
+
+    node_model = deprecated_model_function(_node_model, "node_model")


The _node_model method already wasn't public, so I'm not sure this is necessary?

nice catch!

huonw · 2020-03-25T02:19:08Z

demos/node-classification/gcn/gcn-cora-node-classification-example.ipynb

@@ -509,7 +509,7 @@
    }
   ],
   "source": [
-    "x_inp, x_out = gcn.build()\n",
+    "x_inp, x_out = gcn.in_out_tensors()\n",


There's text above that should be updated via the `GCN.build` method: -> via the `GCN.in_out_tensors` method:.

nice catch! I found another one in the cluster gcn demo

huonw · 2020-03-25T02:21:41Z

tests/layer/test_hinsage.py

@@ -270,41 +270,6 @@ def test_hinsage_apply():
    assert actual == pytest.approx(expected)


-def test_hinsage_default_model():


Why delete this test? Despite the name, it was using build?

ah I was too hasty to delete this one, I'll ad it back and rename it

kjun9 · 2020-03-25T02:21:11Z

tests/layer/test_hinsage.py

@@ -270,41 +270,6 @@ def test_hinsage_apply():
    assert actual == pytest.approx(expected)


-def test_hinsage_default_model():


Should we be trying to keep/update this test?

def test_hinsage_in_out_tensors(): ... xin, xout = hs.in_out_tensors() ...

I was too hasty to delete this one, I'll ad it back and rename it

kjun9

Looks good to me!

huonw

Thanks for doing this so efficiently! Hopefully the in_out_tensors name is clearer 🤞

You'll need to merge with develop to resolve the conflicts caused by the rename in #1153.

kieranricardo · 2020-03-25T02:56:48Z

thanks for your speedy reviews @kjun9 and @huonw!

kieranricardo added 2 commits March 24, 2020 12:02

update layer

f25d22e

updated tests and docs

b7398c4

kieranricardo and others added 4 commits March 24, 2020 12:22

replace .build() with .in_out_tensors()

a0db91d

more changes of build to in_out_tensors + fix attri2vec test

e55e080

replace default_model with in_out_tensors in hateful twitters demo

aabcce8

deprecate default_model and remove unit test

57007bf

kieranricardo marked this pull request as ready for review March 24, 2020 04:24

add mutliplicity keyword arg

dc4d541

kieranricardo requested review from kjun9 and huonw March 24, 2020 04:31

kjun9 reviewed Mar 24, 2020

View reviewed changes

kieranricardo added 2 commits March 24, 2020 17:41

deprecated link_model, node_model, and build with failing full …

9926fed

…batch tests

formatting

863475e

huonw reviewed Mar 24, 2020

View reviewed changes

huonw mentioned this pull request Mar 24, 2020

Switch PendingDeprecationWarnings to real DeprecationWarnings #1145

Merged

huonw added a commit that referenced this pull request Mar 24, 2020

Switch PendingDeprecationWarnings to real DeprecationWarnings (#1145)

8132761

The `default_model` methods are really truly deprecated in favour of `build` (at the moment: #1140, #1089).

kieranricardo added 6 commits March 25, 2020 11:37

deprecated functions statically created

a6dcc32

reverted default_model to pending deprecation

e32e231

unit tests for deprecated functions

b8aff6e

added graph classification to test

c70e9bd

Merge branch 'develop' into enhancement/1089-build-method-clarity

07bdf76

# Conflicts: # demos/node-classification/gcn/gcn-cora-node-classification-example.ipynb # stellargraph/layer/deep_graph_infomax.py # tests/layer/test_deep_graph_infomax.py # tests/layer/test_knowledge_graph.py

develop merge and fixes

7932887

kieranricardo requested review from huonw and kjun9 March 25, 2020 01:44

huonw reviewed Mar 25, 2020

View reviewed changes

kjun9 reviewed Mar 25, 2020

View reviewed changes

demo and docstring .build -> .in_out_tensors change

f028802

huonw mentioned this pull request Mar 25, 2020

Fill out the changelog for work since 0.10 #1151

Merged

kieranricardo added 2 commits March 25, 2020 13:48

added test_hinsage_in_out_tensors

88a0301

removed node_model from RGCN

b95d87f

kieranricardo requested review from huonw and kjun9 March 25, 2020 02:53

kjun9 approved these changes Mar 25, 2020

View reviewed changes

huonw approved these changes Mar 25, 2020

View reviewed changes

Merge branch 'develop' into enhancement/1089-build-method-clarity

0e8865d

huonw added a commit that referenced this pull request Mar 25, 2020

Update for #1140

42d45f3

kieranricardo merged commit 8971299 into develop Mar 25, 2020

kieranricardo deleted the enhancement/1089-build-method-clarity branch March 25, 2020 03:20

kjun9 mentioned this pull request Mar 31, 2020

Feature/node2vec for issue Word2Vec in StellarGraph #255 #536

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement/1089 build method clarity #1140

Enhancement/1089 build method clarity #1140

kieranricardo commented Mar 24, 2020 •

edited by huonw

codeclimate bot commented Mar 24, 2020 •

edited

kjun9 left a comment

huonw commented Mar 24, 2020

kieranricardo commented Mar 24, 2020

kieranricardo commented Mar 24, 2020

kieranricardo commented Mar 24, 2020

huonw commented Mar 24, 2020

huonw Mar 24, 2020 •

edited

kieranricardo Mar 25, 2020

kieranricardo Mar 25, 2020

huonw Mar 25, 2020

kieranricardo Mar 25, 2020

kieranricardo Mar 25, 2020

huonw left a comment

huonw Mar 25, 2020

huonw Mar 25, 2020

kieranricardo Mar 25, 2020

huonw Mar 25, 2020

kieranricardo Mar 25, 2020

huonw Mar 25, 2020

kieranricardo Mar 25, 2020

kjun9 Mar 25, 2020

kieranricardo Mar 25, 2020

kjun9 left a comment

huonw left a comment

kieranricardo commented Mar 25, 2020

		@@ -270,41 +270,6 @@ def test_hinsage_apply():
		assert actual == pytest.approx(expected)


		def test_hinsage_default_model():

Enhancement/1089 build method clarity #1140

Enhancement/1089 build method clarity #1140

Conversation

kieranricardo commented Mar 24, 2020 • edited by huonw

codeclimate bot commented Mar 24, 2020 • edited

kjun9 left a comment

Choose a reason for hiding this comment

huonw commented Mar 24, 2020

manual build methods

defining a single function and setting it in each class

defining a base class for models with a build method

kieranricardo commented Mar 24, 2020

kieranricardo commented Mar 24, 2020

kieranricardo commented Mar 24, 2020

huonw commented Mar 24, 2020

huonw Mar 24, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huonw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjun9 left a comment

Choose a reason for hiding this comment

huonw left a comment

Choose a reason for hiding this comment

kieranricardo commented Mar 25, 2020

kieranricardo commented Mar 24, 2020 •

edited by huonw

codeclimate bot commented Mar 24, 2020 •

edited

defining a base class for models with a `build` method

huonw Mar 24, 2020 •

edited