Don't pass a status to _reader.GetNext()

[akshaym]

Tensorflow's C API now will now raise this error using SWIG without having to raise it in explicitly in Python.

[nfelt]

What versions of TensorFlow does this apply to? We may want to maintain compatibility with older versions.

[jart]

This GitHub depends on tf-nightly pip package. We might need to wait until tomorrow to merge.

Speaking of which, I'd like to learn about what magic makes the with no longer necessary. Is this thanks to cl/140506546? If so, how does that info propagate into a Python exception?

[akshaym]

The removal of with is based on the same idea as cl/191121016 (my cl/190950450 just expands the scope of the SWIG change).

When a C function with a param "TF_Status* status" is wrapped by SWIG, the generated wrapper will correctly generate a TF_NewStatus(), and pass it into the C function. On exit of the C function, the wrapper will check and raise the appropriate error.

[jart]

Oh fabulous. "Raise exception in SWIG on bad TF_Status from C API" (cl/191121016) looks like it'll have asteroid impact. It was however committed a few hours ago. You can move forward with your CL. We'll merge this tomorrow.

[akshaym]

Apologies for the misunderstanding: that change "Raise exception in SWIG on bad TF_Status from C API" will not actually break any of tensorboard.

My change (Apply "Raise exception in SWIG on bad TF_Status" to base.i) which is not yet submitted will break tensorboard without this PR, but I won't be submitting my change till Monday most likely. So hopefully this will get submitted after.

[nfelt]

@akshaym will this change mean that GetNext() will actually forbid a status argument, or will it just ignore it? Only asking since if the status argument is only deprecated it could be useful to leave this in place so users of older TF versions can use newer TB versions (we don't promise this but if it's easy to do, it seems worthwhile).

[akshaym]

@nfelt Unfortunately, it will forbid it.

[chihuahua]

Based on test output, it seems like we're still awaiting a release of tf-nightly?

[akshaym]

Yes, this is still awaiting a release. Hopefully by tomorrow.

[nfelt]

@akshaym maybe we could check the arguments to GetNext() here and condition the behavior on whether it has a status argument? E.g. as follows. That would unblock this PR and also mean we preserve at least some support for TF <= 1.7, which would be nice since users often have to stick with old TF versions but want to upgrade TensorBoard.

import inspect
...
if inspect.getargspec(_reader.GetNext).args[1:]:
    self._reader.GetNext()
else:
    # GetNext() expects a status argument on TF <= 1.7
    with tf.errors.raise_exception_on_not_ok_status() as status:
        self._reader.GetNext(status)

[akshaym]

@nfelt Done. Thanks!

[nfelt]

Does checking for length 0 work? When i was looking at this, "self" is considered the first argument (even though it's a bound function):

>>> inspect.getargspec(reader.GetNext).args
['self', 'status']

Oops, went back to look at my example and I got the negation wrong, my bad. I should have said "not args[1:]" but at that point an explicit length check is probably clearer.

[nfelt]

Also looks like pylint is complaining about getargspec being deprecated on py3.6+. For now probably fine to just suppress deprecated-method warning on this line. I think getfullargspec() is the replacement, but it's not available on Python 2...

[akshaym]

Thanks yeah - I saw that - I also assumed the code works fine (with the "is 0" check) since it didn't complain about this.

[nfelt]

I think it didn't complain because right now with existing tf-nightly len(args) == 2 so this branch isn't triggered. But when tf-nightly picks up your change, len(args) will be == 1 (since "self" will still count as an argument), so this check will still fail, even though the intent is that it should pass. So I think this should be if len(args) == 1: # only the "self" argument to anticipate that change.

[nfelt]

Thanks for making the change backwards-compatible!

[akshaym]

Thanks very much for your help with this!

[jgberg]

When will this go into 1.7 update?

[jart]

Since it's backwards compatible, I don't see any reason why it can't be cherry-picked, to help out our friends who build TF at HEAD.

[nfelt]

@jgberg we could do a cherry-pick if you need one, but note that when building TF at HEAD (or using tf-nightly) it's best to use tb-nightly, the TensorBoard equivalent, rather than the specific TensorBoard releases.

[jgberg]

Thanks! Though I may revert to tf1.6 as my team and I are juggling many things. Nojetheless, others may well benefit if you do. It’s bitten others on our team just to say it... the issue may be widespread...

…

On Tue, Apr 10, 2018 at 11:14 PM Nick Felt ***@***.***> wrote: @jgberg <https://github.com/jgberg> we could do a cherry-pick if you need one, but note that when building TF at HEAD (or using tf-nightly) it's best to use tb-nightly, the TensorBoard equivalent, rather than the specific TensorBoard releases. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1086 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAQh9k_X4jyEE4pY1gAWShClyzgFlQuOks5tnZ9CgaJpZM4TCRXm> .

[sangjeedondrub]

replacingtensorboard/backend/event_processing/event_file_loader.py with lateset one works

[jgberg]

Thanks Sangjee, though I've found that installing Tensorboard v1.8 on top of TF v1.7 and smiling while ignoring the complaints has worked for my purposes...

…

On Thu, May 3, 2018 at 6:03 AM, Sangjee Dondrub ***@***.***> wrote: replacingtensorboard/backend/event_processing/event_file_loader.py with lateset one works — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1086 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAQh9iDeFL2G2cLr6pxTtaFM4lH7qi5Tks5tuwAzgaJpZM4TCRXm> .