Skip to content

Encode run and tag names #480

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 2 commits into from
Sep 6, 2017
Merged

Conversation

chihuahua
Copy link
Member

@chihuahua chihuahua commented Sep 6, 2017

This change encodes the names of runs and tags included within the GET
parameters of URIs for the audio, image, and pr_curve plugins. We pass
those run and tag names into encodeURIComponent.

For the audio and image plugins, we simply needed to use
pluginRunTagRoute while constructing URLs. pluginRunTagRoute calls
queryEncoder from urlPathHelpers, which in turn calls
encodeURIComponent. The pr_curves plugin lacks that luxury because
its URLs contain multiple run GET property-value pairs, and
pluginRunTagRoute only supports 1 run argument. The scalars plugin
might seem analogous, but the scalars plugin actually requests data for
each run--tag combination separately.

Fixes #477. I verified that the audio, image, and PR curve plugins WAI.

This change encodes the names of runs and tags included within the GET
parameters of URIs for the audio, image, and pr_curve plugins. We pass
those run and tag names into encodeURIComponent.

Fixes #477. I verified that the audio, image, and PR curve plugins WAI.
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.

This change looks good to me.

I propose the following long-term plan:

  1. Reinstate addParams(url: string, params: {[name: string]: Param}): string in the urlPathHelpers module. I suspect that we will want
    type Param = string | string[]. This function should include the
    logic to deal with whether a ? is in the string already, the URI
    component encoding, and expanding repeated parameters.

    I say “reinstate” because I added this a long while ago, but wasn’t
    able to merge it because of junk with the router’s “demo mode”
    feature. But our new versions of the plugins don’t care about demo
    mode at all, so this shouldn’t be a problem. (If it is, we should
    first remove all mentions of demo mode.)

  2. Remove pluginRunTagRoute. I see your change in the image loader
    logic
    , and I understand why you did it, and I agree that is a fine
    approach for right now. But there is an aspect of the former code
    that I prefer: all the parameter names appear in one place. Here,
    we have run and tag being added by pluginRunTagRoute, while
    sample is added later. This isn’t a huge deal, but it’s an
    unnecessary layer of confusion.

    On a more fundamental level, pluginRunTagRoute is a bit of legacy
    code. A long time ago we had methods backend.scalar(run, tag)
    and backend.scalarTags(). I introduced pluginRunTagRoute so that
    we had an immediate compatibility layer for forms like the latter.
    But our router backend shouldn’t presume anything about the URL
    format of plugins. This function has too low of a power-to-weight
    ratio.

What do you think?

@chihuahua
Copy link
Member Author

Those are really good points.

  1. addParams seems broadly applicable to and clear to plugin developers. @jart, can we drop support for demo mode? It seems like we can. Lets reinstate addParams, then ensure ... um ... the internal tool for managing experiments still works.

  2. Yeah, addParams would seem to obviate the need for pluginRunTagRoute. It does seem arbitrary for the backend to have both pluginRoute and pluginRunTagRoute. pluginRoute should suffice.

@chihuahua chihuahua merged commit 0f23598 into master Sep 6, 2017
@chihuahua chihuahua deleted the chihuahua-encode-runs-and-tags branch September 6, 2017 18:41
@wchargin
Copy link
Contributor

wchargin commented Sep 6, 2017

Our internal tool for managing experiments does not use demo mode. I looked into deleting it once and it was a pain, but it should be easier now if you want to try.

@wchargin
Copy link
Contributor

wchargin commented Sep 6, 2017

I'll do the other two items.

jart pushed a commit to jart/tensorboard that referenced this pull request Sep 6, 2017
This change encodes the names of runs and tags included within the GET
parameters of URIs for the audio, image, and pr_curve plugins. We pass
those run and tag names into encodeURIComponent.

Fixes tensorflow#477. I verified that the audio, image, and PR curve plugins WAI.
jart pushed a commit that referenced this pull request Sep 6, 2017
This change encodes the names of runs and tags included within the GET
parameters of URIs for the audio, image, and pr_curve plugins. We pass
those run and tag names into encodeURIComponent.

Fixes #477. I verified that the audio, image, and PR curve plugins WAI.
@wchargin
Copy link
Contributor

wchargin commented Sep 6, 2017

Done in #490 #492 #491 #494.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants