Skip to content

Conversation

@bileschi
Copy link
Collaborator

@bileschi bileschi commented Feb 29, 2020

Cherrypicks:

The bulk of these are feature additions to the uploader to support
experiment name and description metadata. This also includes
compatibility changes to use tf_record_iterator rather than SWIG
bindings.

This release has new features, but is a patch release in order to
maintain cadence with TensorFlow.

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@bileschi bileschi changed the title 2.1 Cherry picks to 2.1 release branch Feb 29, 2020
@wchargin
Copy link
Contributor

Let’s make sure to merge with rebase, not squash.

@wchargin
Copy link
Contributor

Build broken; #3311 is an immediate fix (can cherry-pick). TensorFlow’s
public Python 2 policy
is that patch releases will not have Python 2
packages, so we should also feel empowered to drop Python 2 support with
TensorBoard 2.1.1, but it may end up being simpler to just continue with
the existing build/CI/PyPI setup for this patch release. Either is fine
with me.

Copy link
Contributor

@nfelt nfelt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, could we change the first commit (g-import-not-at-top) so that we just apply the subset that affects the uploader tree? I'd be a little happier if we could avoid the delta to all the other files outside /uploader even though I realize the change is comment-only.

cc @wchargin for your thoughts

@wchargin
Copy link
Contributor

Hmm, could we change the first commit (g-import-not-at-top) so that we
just apply the subset that affects the uploader tree?

That would be fine with me.

@bileschi: One way to do this would be to interactively rebase the
branch, change the pick b152de4a pylint: ... line on the second line
of the rebase plan to edit b152de4a pylint: ..., run

git checkout HEAD^ ':!tensorboard/uploader/`

to check out the old versions of all the non-uploader code, then run
git commit --amend to update the commit (and preferably add a short
message indicating what was changed), then git rebase --continue.
You’ll need to git push --force-with-lease bileschi 2.1 after
rewriting those commits.

wchargin and others added 4 commits March 2, 2020 16:27
Summary:
A new year, a new dawn. The <https://pythonclock.org/> has expired, and
TensorFlow has dropped Python 2 support:
<https://groups.google.com/a/tensorflow.org/d/msg/developers/ifEAGK3aPls/jlEuWYObBgAJ>

As noted in the above announcement, TensorFlow 2.1.0 will be released
with Python 2.7 support, but we’ve already released the corresponding
TensorBoard 2.1.0, so we no longer need to test with Python 2 at head.

Test Plan:
That CI passes quickly suffices.

wchargin-branch: travis-nopy2
Although the werkzeug changelog did not say so, it seem to have removed
certain symbols from `werkzeug`. We now use
`werkzeug.wrappers.BaseResponse` instead.

This is a cherry-pick of f833b39 to
avoid black reformatting.

Context:
https://werkzeug.palletsprojects.com/en/master/changes/#version-1-0-0
Summary:
This is a Google-specific lint that we kind of just cargo cult around
and don’t actually lint against in CI, which makes it less than useful.

Generated with:

```
git ls-files -z '*.py' |
    xargs -0 sed -i \
        -e 's/g-import-not-at-top,//' \
        -e 's/,g-import-not-at-top//' \
        -e 's/ *# pylint:.*=g-import-not-at-top$//' \
        ;
```

Note: This cherry-pick only includes files under tensorboard/uploader
28 files from the original commit were omitted.

Test Plan:
Our standard lint still passes:

```
$ flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics
0
```

wchargin-branch: unsuppress-nontop-imports
@wchargin
Copy link
Contributor

wchargin commented Mar 3, 2020

Please cherry-pick #3320 to fix Python 3.5 release. (Verified fixed in a
Python 3.5 virtualenv with TensorBoard from head and TensorFlow at
either tf-nightly or tensorflow==2.1.0.)

@nfelt
Copy link
Contributor

nfelt commented Mar 3, 2020

Let's also cherry pick #3185 (fixes this release to work with upcoming TF 2.2.0+) and #3319 (followup to that PR that avoids console spam).

tensorboard-gardener and others added 15 commits March 3, 2020 16:02
…pec. (tensorflow#3116)

* When available, use inspect.getfullargspec instead of
inspect.getargspec.

The former is only available in Python3, in which getargspec was
deprecated. This allows to get rid of a deprecation warning.

Both return a namedtuple with an equivalent "args" attribute, so the
functionality is unchanged.

* Changes so that black --check succeeds.
…rflow#3185)

* modernize event_file_loader_test.py and improve coverage

* update EventFileLoader to use tf_record_iterator when possible

* remove legacy TF <=1.6 GetNext() support

* CR: remove unused tempfile import
Summary:
Follow-up to tensorflow#2978, which added explicit configuration of `TZ=UTC`. In
cPython 3, the time zone used by `datetime.datetime.fromtimestamp` is
[cached when `time` is imported][1] unless explicitly reset, so mocking
the environment variable is no longer sufficient. We now call `tzset`
ourselves; we could also just set `TZ=UTC` before importing `datetime`,
but this seems cleaner.

[1]: https://github.com/python/cpython/blob/2528a6c3d0660c03ae43d796628462ccf8e58190/Modules/timemodule.c#L1752-L1763

Test Plan:
A test sync shows that this test passes in both Python 2 and Python 3,
whereas it previously only passed in Python 2.

wchargin-branch: util-test-tzset
* add lint check against BUILD targets with python_version = "PY2"

* try to fix syntax

* add comment about literal

* fix yaml lint

* remove python_version = "PY2" from tensorboard/uploader:uploader
)

Proto changes to support experiment name & description.
…3236)

* Update export/write services to add needed fields & calls for eventual tensor storage
…sorflow#3255)

* copy export_service.proto to experiment.proto

* Extract Experiment/ExperimentMask messages into experiment.proto
* Adds name and description to the output of "list" subcommand

* Adds name and description as upload options to the uploader

* add subcommand for update-metadata to change name and description

* black

* adds additional testing.  Fixes a logging error

* Adds additional testing.  Changes print output to log.info

* black

* stray keystroke

* black

* Adds test for INVALID_ARGUMENT.  Addresses more reviewer critiques

* black & stray edit

* evenblacker
Summary:
This enables us to flag features on the server side, so that we can
stage releases, and so that we can roll back in a pinch without
requiring an expedited PyPI push and end user version upgrade.

Test Plan:
No new behavior in this change. The protos still build.

wchargin-branch: uploader-plugincontrol-protos
wchargin and others added 10 commits March 3, 2020 16:16
Summary:
Tests create uploaders and request senders with dependency-injected
stubs. As we add more such stubs, the test code bloats and requires
broad changes. By centralizing helpers, we can inject defaults that are
selectively overridden.

Test Plan:
Existing tests suffice.

wchargin-branch: uploader-test-di-helpers
Summary:
When the server sends a `PluginControl` message in the initial
handshake, we now only send data for plugins specified therein.

Test Plan:
Unit tests included. As end-to-end tests:

  - When running against prod (which does not send `plugin_control`),
    experiment upload is unchanged. Info logs now include “Skipping time
    series […] with unsupported plugin name 'histograms'”, just once per
    time series.

  - When running against a local server that serves scalars-only
    explicitly—

    ```
    plugin_control {
      allowed_plugins: "scalars"
    }
    ```

    —the behavior is also the same.

  - When running against a local server that serves “no plugins allowed”
    explicitly—

    ```
    plugin_control {
    }
    ```

    —nothing is uploaded, and the logs include “Skipping…” messages for
    scalar time series.

wchargin-branch: uploader-plugincontrol
Summary:
Recent refactorings to the RPC rate limiting had the unintended side
effect of removing rate limiting on logdir polling. This meant that
after all data had been read from the logdir, the uploader would
continue to poll it aggressively (thousands of times per second on my
machine), which is bad for disks and expensive on network file systems.

This commit adds a separate rate limiter for the logdir polling. We
don’t reuse the RPC rate limiter because that would force us to tick
twice on the same timer every cycle.

Fixes tensorflow#3301.

Test Plan:
Run `bazel run //tensorboard -- dev upload --logdir /nope --verbosity 0`
(with any logdir path, but it’s easier when it’s empty), and note that
the “Starting an upload cycle” logs now progress at 0.2 per second
rather than 6000 per second. Unit tests also included: if the `.tick()`
is removed, they fail (quickly, without hanging).

wchargin-branch: uploader-rate-limit-polling
Summary:
We now emit an experiment’s scalar data to `experiment_123/scalars.json`
rather than `scalars_123.json`. This generalizes more cleanly to writing
multiple files under an experiment’s directory.

Test Plan:
Unit tests updated. One previous test case can no longer be hit and has
thus been removed.

wchargin-branch: uploader-export-subdirs
Summary:
This `f`-string has no interpolations, so it *can* be a normal string,
and the Bazel target is marked `srcs_version = "PY2AND3"`, so it *must*
be a normal string.

Test Plan:
This module now parses under Python 2:

```
python2 -c '__import__("ast").parse(__import__("sys").stdin.read())' \
    <tensorboard/uploader/uploader_main.py
```

wchargin-branch: uploader-remove-fstring
Summary:
The `StreamExperiments` RPC response used to send only `experiment_ids`,
but now sends `experiments` with additional metadata. This server-side
change has been live since late November 2019, so we’re confident that
we won’t need to roll it back. Thus, we can drop compatibility for the
old code path in new uploader clients.

Test Plan:
Unit tests updated; verified that the `list` and `export` subcommands
still work.

wchargin-branch: uploader-require-experiment-metadata
Summary:
When running `tensorboard dev export`, experiment names and descriptions
(set with the `update-metadata` subcommand or the `--name` and
`--description` flags to `upload`) as well as experiment creation and
modification times are now emitted to a new `metadata.json` file in the
experiment directory.

Test Plan:
Unit tests included. Also end-to-end tested against a live server,
exporting some experiments with name/description and some without.

wchargin-branch: uploader-export-metadata
@bileschi
Copy link
Collaborator Author

bileschi commented Mar 3, 2020

All CPs were googler-authored.

@bileschi bileschi added cla: yes and removed cla:yes labels Mar 3, 2020
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@wchargin wchargin changed the title Cherry picks to 2.1 release branch TensorBoard 2.1.1 Mar 3, 2020
Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval to merge with rebase.

@nfelt nfelt merged commit 66d10d7 into tensorflow:2.1 Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants