Rip in TFDS, I'm gonna try to get that generic dummy data up and running

@AdamHillier I know this is my PR, but could you update this to use #119 ? I can also do it myself, probably just not today

I don't think I like having the model summary strings in tests/snapshots/snap_models_test.py. If we put them all in separate .txt files (named by their models), it'll be easier to track model changes over time and find/link to individual model summaries.

I don't think I like having the model summary strings in tests/snapshots/snap_models_test.py. If we put them all in separate .txt files (named by their models), it'll be easier to track model changes over time and find/link to individual model summaries.

This is not a choice I've made; it's just how snapshots work. I'm not sure if what you suggest is possible without creating an explicit python test file for every model separately...

Edit: obviously it is possible, but then we'd need to not use snapshot and manually save, load and compare them. Sounds like a lot of hassle to me but could be okay. I also don't like the way snapshot works; if a comparison is wrong it will just print the entire string in red without much of an indication of what's wrong.

Ah, I see. Since we're literally just comparing the string output of model.summary() to the text contents of a .txt file though, it shouldn't be too hard to implement without the snapshottest framework right?

Ah, I see. Since we're literally just comparing the string output of model.summary() to the text contents of a .txt file though, it shouldn't be too hard to implement without the snapshottest framework right?

Yes true, I think I like this suggestion

The nice thing about the snapshot module is that there is a single pytest command to update the snapshot. As long as there is a nice way to replicate that with a single command, I'm happy to have individual txt files.

The nice thing about the snapshot module is that there is a single pytest command to update the snapshot. As long as there is a nice way to replicate that with a single command, I'm happy to have individual txt files.

As long as we keep these summaries in a specific folder, we can just delete the folder and re-generate the snapshots that way?

As long as we keep these summaries in a specific folder, we can just delete the folder and re-generate the snapshots that way?

Sure, if that's how it works then that sounds good.

I like this, once the isort linting is fixed should be good to go.

😍 love this!

@larq/core For some reason, keras.backend.clear_session() (called in parametrize) no longer seems to have any effect; the second model to be tested will have layer names like input_2 etc. Does anyone have any idea what might be the issue here? I'm on TF 2.0.0 and manually calling clear_session() makes no difference...

On February 27th, this was working fine so something must have changed

It also looks like it's failing on fixture 'snapshot' not found. Maybe some requirements/imports got messed up when you merged master?

It also looks like it's failing on fixture 'snapshot' not found. Maybe some requirements/imports got messed up when you merged master?

Yeah that's indeed interesting, though snapshottest is still in the requirements and pytest is imported so I don't see why that wouldn't work...

Hopefully CI for this should pass now that #166 / #168 are merged.

There's surely something wrong with 018ed29, Larq Zoo shouldn't be on its own line should it?

There's surely something wrong with 018ed29, Larq Zoo shouldn't be on its own line should it?

Why not? It's not a third-party library in this case

Why not? It's not a third-party library in this case

Sorry you're absolutely right, that was a dumb moment on my part 😂

So this looks like it's still failing now because auto-named layers have the wrong indices because previously-built models aren't properly being cleared out? So same problem as before :(

Haha so it turns out that this code actually works just fine, it's just that the snapshots must have been generated when it wasn't working because the layer names in the .txt files are definitely wrong, they just need to be re-generated :)

I'm about to push a commit that does that.

Well that's progress, but unfortunately it looks like TF 2.2 changes a default layer name, tf_op_layer_Mul -> tf_op_layer_mul. Not sure how we can get around that without setting layer names explicitly....

Well that's progress, but unfortunately it looks like TF 2.2 changes a default layer name, tf_op_layer_Mul -> tf_op_layer_mul. Not sure how we can get around that without setting layer names explicitly....

I compare the summary strings manually so we could just use assert snapshot.lower() == current.lower()... It's not extremely pretty but I don't think it's necessarily a bad solution either; in general this makes the snapshots less brittle (I don't foresee any scenario in which this would cause issues)

Nice, exciting that this is ready now!

@lgeiger I think this should be good to go, could you review this another time? Merging is currently blocked.

@larq/core do we still want this? If so, I'll resolve the conflicts and we can get this merged. If not, let's close this PR.

@larq/core do we still want this? If so, I'll resolve the conflicts and we can get this merged. If not, let's close this PR.

I don't have a strong desire for this, and it seems like it'd be a bunch of work to get it working, so I'd be minded to close it.

@larq/core do we still want this? If so, I'll resolve the conflicts and we can get this merged. If not, let's close this PR.

I'm fairly neutral to this, although I fear that these tests might be tricky to maintain accross the multiple versions of TensorFlow which we support since the snapshots might be slightly different depending on the TensorFlow version used to generate them.

Would it make sense to just compare the ModelProfiles instead? Then we don't need to care about the layer names etc, but will at least be notified if e.g, the number of MACs suddenly changes unintentionally

Would it make sense to just compare the ModelProfiles instead? Then we don't need to care about the layer names etc, but will at least be notified if e.g, the number of MACs suddenly changes unintentionally

Would this catch issues that are not already covered by the unit tests for the ModeProfile itself? (https://github.com/larq/larq/blob/master/larq/models_test.py#L66)

Would this catch issues that are not already covered by the unit tests for the ModeProfile itself? (https://github.com/larq/larq/blob/master/larq/models_test.py#L66)

It would do that check for all the models, rather than just for the toy model used in the unit tests. That test is a good way to check the calculations made in ModelProfile are still correct, but the idea behind the tests here is that we'd want to catch subtle things like accidentally enabling / disabling biases somewhere while updating some code

We decided that at this time, it's not worth the extra effort to change to a ModelProfile snapshot solution. I'll close this PR for now, but we can re-open it in the future if we do decide this is necessary.