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 Name Conflicts During Meta Object Reification #93

Open
brandonwillard opened this issue Dec 3, 2019 · 2 comments · May be fixed by #94
Open

TensorFlow Name Conflicts During Meta Object Reification #93

brandonwillard opened this issue Dec 3, 2019 · 2 comments · May be fixed by #94
Assignees
Labels
enhancement New feature or request important Features and issues that need to be addressed ASAP meta graph This issue involves the meta graph objects TensorFlow This issue involves the TensorFlow backend

Comments

@brandonwillard
Copy link
Contributor

An issue arises when reifying meta tensors that are intended to be new to a TF graph, because a tensor with an unspecified name value is currently set—by default—to its OpDefs name. This issue should be easily fixed by using the standard TF means of generating a unique name. I'll put in a fix ASAP.

Originally posted by @brandonwillard in #75 (comment)

@brandonwillard brandonwillard self-assigned this Dec 3, 2019
@brandonwillard brandonwillard added bug Something isn't working important Features and issues that need to be addressed ASAP labels Dec 3, 2019
brandonwillard added a commit to brandonwillard/symbolic-pymc that referenced this issue Dec 3, 2019
`NodeDef` name values can now be `None`, which means that reification
will use the next available unique name in the default graph.

Closes pymc-devs#93.
@brandonwillard brandonwillard linked a pull request Dec 3, 2019 that will close this issue
@brandonwillard
Copy link
Contributor Author

Here's a representative, minimal example of this problem:

import tensorflow as tf

from tensorflow.python.eager.context import graph_mode

from unification import unify, reify, var

from symbolic_pymc.tensorflow.meta import mt


with graph_mode(), tf.Graph().as_default() as test_graph:

    # Divide two constant integers
    first_div_mt = mt(1) / mt(2)

    # Create a TF graph for that division
    first_div_tf = first_div_mt.reify()

    # Create a division "pattern" with variable/unknown dividends and name
    # parameter
    div_lv = mt.realdiv(var('b'), var('c'), name=var('name'))

    # Unify the "pattern" with the TF graph and get the logic variable
    # replacements map/dict
    s = unify(first_div_tf, div_lv)

    # Change the dividends in the substitution map so that the following
    # reification will produce a new new graph
    s[var('b')] = 1
    s[var('b')] = 3

    div_mt = reify(div_lv, s)

    # Attempting to create this TF graph within the same parent graph
    # (i.e. `test_graph`) as `first_div_tf` will throw a
    # `MetaReificationError`, because it would create two graphs with the same
    # name (not possible/expected in TF).
    div_mt.reify()

Before #90, we would allow this, which would result in meta objects with unequal (in name) base objects.

This previous name inconsistency would also lead to unexpected object creation when meta objects were intended to correspond to existing base objects, but some step along the way unintentionally—say—altered a meta object property and reasonably broke the correspondence. In this case, the added name consistency check serves as a more direct means of addressing these scenarios.

The example above is not necessarily inconsistent, though. The problem in that example is, however, easily remedied. Here are two basic approaches:

with graph_mode(), test_graph.as_default():
    # Reify an equivalent "pattern" that uses the same logic variables, save
    # the name, e.g.
    div_mt = reify(mt.realdiv(var('b'), var('c')), s)

    # No problem here
    div_mt.reify()


with graph_mode(), test_graph.as_default():
    # Manually change the name value in the replacements map, e.g.
    s[var('name')] = "new_name"
    div_mt = reify(div_lv, s)

    # No problem here
    div_mt.reify()

Of the two, the first one is most "compatible" with miniKanren, since it can be implemented without resorting to a custom, low-level goal that (non-relationally) manipulates the state/replacement mappings.

Still, it seems like the original example should work in the same way that specifying an existing tensor name does in the TF API (i.e. it creates the next unique name suffixed with an underscore and number).

One way to do this would involve a succinct means of specifying that the names in the "pattern" graph, div_lv, are not a strict assignment.

@brandonwillard brandonwillard added enhancement New feature or request and removed bug Something isn't working labels Dec 3, 2019
@brandonwillard
Copy link
Contributor Author

I've removed the bug label, because I can't find an actual error caused by this behavior. It's really just "unintuitive" at the user-level and will likely trip people up.

brandonwillard added a commit to brandonwillard/symbolic-pymc that referenced this issue Dec 4, 2019
When the default tensor string name type, `DefaultTensorName`, is used, the
`TFlowMetaTensor.reify` will not assume that the string name is "strict" and
rely upon the base graph to assign a unique name derived from the default one.

Closes pymc-devs#93.
@brandonwillard brandonwillard changed the title TF Name Conflicts During Meta Object Reification TensorFlow Name Conflicts During Meta Object Reification Dec 4, 2019
@brandonwillard brandonwillard added TensorFlow This issue involves the TensorFlow backend meta graph This issue involves the meta graph objects labels Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request important Features and issues that need to be addressed ASAP meta graph This issue involves the meta graph objects TensorFlow This issue involves the TensorFlow backend
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant