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

SameDiff If, While, and Misc changes #52

Merged
merged 13 commits into from Jul 12, 2019

Conversation

@rnett
Copy link
Collaborator

commented Jul 9, 2019

Main changes: Add tensorflow style If and While, and methods. Backprop is not implemented, and While has issues with nesting external variables (Enter I think). Backprop is being punted and I'll open an issue for it, unsure about Enter.

Also fixes eclipse#7981 by adding checks for already having name scope in multiple places.

Adds ArgumentInterceptor for detecting the use of tensors within a scope. Also adds a argument replacement method (which doesn't use ArgumentInterceptor).

Changes op names to use '_' instead of ':' when there are name conflicts (otherwise we get var names like while/add:1:1)

Adds logSoftmax and softmax with dimension, adds any and all.

Adds tests for If and While, and for name scoping.

rnett added 7 commits Jul 4, 2019
softmax and logSoftmax w/ dimension
Signed-off-by: Ryan Nett <rnett@skymind.io>
start of while
Signed-off-by: Ryan Nett <rnett@skymind.io>
if, start of javadocs
Signed-off-by: Ryan Nett <rnett@skymind.io>
while foreward pass working, backprop WIP
Signed-off-by: Ryan Nett <rnett@skymind.io>
no backprop
Signed-off-by: Ryan Nett <rnett@skymind.io>
Tensorflow style if/while (& tests), name scope fixes (and test), arg…
…ument interceptor (for if/while), use '_' in op names instead of ':'

Signed-off-by: Ryan Nett <rnett@skymind.io>
javadoc
Signed-off-by: Ryan Nett <rnett@skymind.io>

@rnett rnett requested a review from AlexDBlack Jul 9, 2019

@AlexDBlack
Copy link

left a comment

Overall logic looks good.
We'll refactor the logic out of NewIf etc classes into methods instead.
Otherwise just minor nitpicks/polish, and some extra tests.

@rnett

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 10, 2019

Serialization/deserialization tests are failing with java.lang.IllegalStateException: java.lang.UnsupportedOperationException: Unable to map op 100 of type META for while and java.lang.IllegalStateException: java.lang.UnsupportedOperationException: Unable to map op 30 of type META for If. This is after moving to methods, so its not my ops causing this.

@AlexDBlack thoughts? I'm guessing it has to do with enter or switch, is there anything I have to do to them to implement serialization?

many fixes
Signed-off-by: Ryan Nett <rnett@skymind.io>
@AlexDBlack
Copy link

left a comment

Minor issues, plus a bit more polish required, but this is looking pretty good otherwise. 👍

As for "Serialization/deserialization tests are failing" - post full stack trace?
Probably just an issue with FlatBuffers mapping for deserialization at a guess... which is interesting, because TF import uses these ops, and those are always tested for ser/de...

condScope.close();

if (pred.dataType() != DataType.BOOL)
throw new IllegalStateException("Can not use " + pred.getVarName() + " as the condition of an If statement, the condition must be a boolean.");

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Jul 11, 2019

One thought here (and while) - and maybe it's not a big deal right now - is that when throwing this exception, we basically leave the SameDiff instance in a broken state - ifScope is still open, and we've got a partially defined if (added cond, but nothing else).
Fixing that might be difficult, but maybe we log an issue on that as a "nice to have" thing to fix in the future.

This comment has been minimized.

Copy link
@rnett

rnett Jul 11, 2019

Author Collaborator

As a general thing, it seems like you would have to do some DBMS style transaction with commit/abort, which seems like overkill. As for this, if there is a way to get all ops/variables within a name scope, it would be fairly easy to get them and remove them, and then close scope, before throwing the error.

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Jul 11, 2019

Yeah, I agree it's overkill for now.
No way to get all variables in a scope at present, but that should be pretty easy to add (just check name prefix, essentially). If it's not too difficult, let's remove them. I don't like the idea of leaving people with broken nets (you can imagine using SameDiff in a notebook or something - try to add loop, get exception, fix it, and try again on broken net -> bad times).

@rnett

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 11, 2019

Test error stack trace:

java.lang.IllegalStateException: java.lang.UnsupportedOperationException: Unable to map op 30 of type META

	at org.nd4j.autodiff.samediff.SameDiffTests.testIf(SameDiffTests.java:3480)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	...
Caused by: java.lang.UnsupportedOperationException: Unable to map op 30 of type META
	at org.nd4j.autodiff.samediff.serde.LegacyOpMapper.getLegacyOpClassForId(LegacyOpMapper.java:136)
	at org.nd4j.autodiff.samediff.serde.FlatBuffersMapper.fromFlatNode(FlatBuffersMapper.java:393)
	at org.nd4j.autodiff.samediff.SameDiff.fromFlatBuffers(SameDiff.java:5226)
	at org.nd4j.autodiff.samediff.SameDiff.fromFlatBuffers(SameDiff.java:5147)
	at org.nd4j.autodiff.samediff.SameDiffTests.testIf(SameDiffTests.java:3474)
	... 36 more
many fixes
Signed-off-by: Ryan Nett <rnett@skymind.io>

@rnett rnett requested a review from AlexDBlack Jul 11, 2019

@AlexDBlack

This comment has been minimized.

Copy link

commented Jul 11, 2019

Re: stack trace - looks like the problem is here:
https://github.com/eclipse/deeplearning4j/blob/b5f0ec072f3fd0da566e32f82c0e43ca36553f39/nd4j/nd4j-backends/nd4j-api-parent/nd4j-api/src/main/java/org/nd4j/autodiff/samediff/serde/FlatBuffersMapper.java#L368-L394

So only custom ops and legacy ops are set up for proper deserialization. Not ENTER etc.
I'm surprised we haven't run into that already.

It might be as simple as if(opType == Op.Type.CUSTOM || opType == Op.Type.ENTER || ...) {
If not, we might need an extra else/if there to handle the deserialization.

Some fixes
Signed-off-by: Ryan Nett <rnett@skymind.io>
@rnett

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 11, 2019

So, it seems that Switch (and possibly others) have the correct Op.Type, but are being saved using OpType.LOGIC, which is then deserialized into Op.Type.META (which is incorrect and causes the error).

It seems like a pretty straight foreward change to add Enter/Exit/etc types to OpType, getFlatOpType, and getTypeFromByte, but is there anything else it needs to match (e.g. c++)?

cleanup if condition doesn't return boolean
Signed-off-by: Ryan Nett <rnett@skymind.io>
@AlexDBlack

This comment has been minimized.

Copy link

commented Jul 12, 2019

OK, so I think FlatBuffersMapper.getFlatOpType returning LOGIC is fine.
So yes, we'll need to change FlatBuffersMapper.getTypeFromByte(byte) to getTypeFromByte(byte, long opNum) and handle those "logic" op cases there. I think that's the easiest solution, and shouldn't impact c++ at all.

serialization fix
Signed-off-by: Ryan Nett <rnett@skymind.io>
@rnett

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 12, 2019

Alright, its fixed. Tests pass (although I haven't tested c++ side if there is one).

@AlexDBlack
Copy link

left a comment

Let's switch those "magic constants" used in a few places to public static final fields for Enter, Exit, etc.
This applies to both the op names and op numbers, for all of these conditional/logic ops.

use constants instead of magic numbers
Signed-off-by: Ryan Nett <rnett@skymind.io>
@AlexDBlack
Copy link

left a comment

LGTM 👍

@rnett rnett merged commit c58e17a into master Jul 12, 2019

@rnett rnett deleted the rn_sd_if_while branch Jul 12, 2019

AlexDBlack added a commit that referenced this pull request Jul 20, 2019
SameDiff If, While, and Misc changes (#52)
* softmax and logSoftmax w/ dimension

Signed-off-by: Ryan Nett <rnett@skymind.io>

* start of while

Signed-off-by: Ryan Nett <rnett@skymind.io>

* if, start of javadocs

Signed-off-by: Ryan Nett <rnett@skymind.io>

* while foreward pass working, backprop WIP

Signed-off-by: Ryan Nett <rnett@skymind.io>

* no backprop

Signed-off-by: Ryan Nett <rnett@skymind.io>

* Tensorflow style if/while (& tests), name scope fixes (and test), argument interceptor (for if/while), use '_' in op names instead of ':'

Signed-off-by: Ryan Nett <rnett@skymind.io>

* javadoc

Signed-off-by: Ryan Nett <rnett@skymind.io>

* many fixes

Signed-off-by: Ryan Nett <rnett@skymind.io>

* many fixes

Signed-off-by: Ryan Nett <rnett@skymind.io>

* Some fixes

Signed-off-by: Ryan Nett <rnett@skymind.io>

* cleanup if condition doesn't return boolean

Signed-off-by: Ryan Nett <rnett@skymind.io>

* serialization fix

Signed-off-by: Ryan Nett <rnett@skymind.io>

* use constants instead of magic numbers

Signed-off-by: Ryan Nett <rnett@skymind.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.