Skip to content

Fix Scope name collisions #248

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

Merged
merged 11 commits into from
May 28, 2021
Merged

Fix Scope name collisions #248

merged 11 commits into from
May 28, 2021

Conversation

rnett
Copy link
Contributor

@rnett rnett commented Mar 20, 2021

This fixes two possible sources of name collision issues:

  • Already existing ops in an imported graph
  • Separate top level NameScope instances

This could probably be done better by either checking against the environment every time, or having a base NameScope instead of scope, but both of those would require large restructurings. This is just a patch to get it working.

@rnett rnett mentioned this pull request Mar 20, 2021
@rnett rnett mentioned this pull request Mar 28, 2021
@rnett rnett mentioned this pull request May 21, 2021
rnett and others added 4 commits May 26, 2021 13:17
@rnett
Copy link
Contributor Author

rnett commented May 26, 2021

I had to add a quick fix for spotless as well, it wouldn't work when called from subdirectories.

@Craigacp
Copy link
Collaborator

The latest commit removes copyright statements. We shouldn't do that.

@rnett
Copy link
Contributor Author

rnett commented May 26, 2021

I took it out as per #209 (comment). Is that incorrect?

Signed-off-by: Ryan Nett <[email protected]>
@Craigacp
Copy link
Collaborator

Craigacp commented May 26, 2021

You can remove it from things where the copyright is owned by TensorFlow contributors, or Google, if that's what they want. But you can't remove it from the code I've contributed as that's under an Oracle copyright, and my lawyers will be grumpy at me. In this case it's not doing that, but if you enforce that on commit or file update it will do, and that would be bad.

Copy link
Collaborator

@karllessard karllessard 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 the way env is passed to the various constructors of NameScope could still be improved but let's merge it the way it is now.

@karllessard karllessard merged commit 3b4533c into tensorflow:master May 28, 2021
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.

3 participants