updating umap version #2898
updating umap version #2898
Conversation
|
@wchargin ptal, thanks |
third_party/js.bzl
Outdated
| @@ -212,8 +212,8 @@ def tensorboard_js_workspace(): | |||
| licenses = ["notice"], # Apache License 2.0 | |||
| sha256_urls = { | |||
| "85a2ff924f1bf4757976aca22fd0efb045d9b3854f5a4ae838c64e4d11e75005": [ | |||
There was a problem hiding this comment.
This PR doesn’t update the file hash, so Bazel will still read the old
version of the code if it’s in your local cache, or fail with a fetch
error otherwise. Please update the checksum to the new value.
Testing that the projector still works, as you described, is
great—thanks! Is there a way that we can check that the new features
were properly integrated? What are the changes in this version bump?
There was a problem hiding this comment.
In particular, this PR introduces the following error on a clean cache
(note the checksum mismatch message):
INFO: Call stack for the definition of repository 'ai_google_pair_umap_js' which is a filegroup_external (rule definition at /HOMEDIR/.cache/bazel/_bazel_wchargin/52a95bbdd50941251730eb33b7476a66/external/io_bazel_rules_closure/closure/filegroup_external.bzl:186:22):
- /HOMEDIR/git/tensorboard/third_party/js.bzl:209:3
- /HOMEDIR/git/tensorboard/third_party/workspace.bzl:35:3
- /HOMEDIR/git/tensorboard/WORKSPACE:98:1
WARNING: Download from http://mirror.tensorflow.org/unpkg.com/umap-js@1.2.2/lib/umap-js.min.js failed: class com.google.devtools.build.lib.bazel.repository.downloader.UnrecoverableHttpException Checksum was 035fede477f10b909dd64a2ea01c031149ee523f54fb9bbe48a170eb04d53825 but wanted 85a2ff924f1bf4757976aca22fd0efb045d9b3854f5a4ae838c64e4d11e75005
WARNING: Download from https://unpkg.com/umap-js@1.2.2/lib/umap-js.min.js failed: class com.google.devtools.build.lib.bazel.repository.downloader.UnrecoverableHttpException Checksum was 035fede477f10b909dd64a2ea01c031149ee523f54fb9bbe48a170eb04d53825 but wanted 85a2ff924f1bf4757976aca22fd0efb045d9b3854f5a4ae838c64e4d11e75005
ERROR: An error occurred during the fetch of repository 'ai_google_pair_umap_js':
java.io.IOException: Error downloading [http://mirror.tensorflow.org/unpkg.com/umap-js@1.2.2/lib/umap-js.min.js, https://unpkg.com/umap-js@1.2.2/lib/umap-js.min.js] to /HOMEDIR/.cache/bazel/_bazel_wchargin/52a95bbdd50941251730eb33b7476a66/external/ai_google_pair_umap_js/umap-js.min.js: Checksum was 035fede477f10b909dd64a2ea01c031149ee523f54fb9bbe48a170eb04d53825 but wanted 85a2ff924f1bf4757976aca22fd0efb045d9b3854f5a4ae838c64e4d11e75005
ERROR: /HOMEDIR/git/tensorboard/tensorboard/components/tf_imports/BUILD:89:1: no such package '@ai_google_pair_umap_js//': java.io.IOException: Error downloading [http://mirror.tensorflow.org/unpkg.com/umap-js@1.2.2/lib/umap-js.min.js, https://unpkg.com/umap-js@1.2.2/lib/umap-js.min.js] to /HOMEDIR/.cache/bazel/_bazel_wchargin/52a95bbdd50941251730eb33b7476a66/external/ai_google_pair_umap_js/umap-js.min.js: Checksum was 035fede477f10b909dd64a2ea01c031149ee523f54fb9bbe48a170eb04d53825 but wanted 85a2ff924f1bf4757976aca22fd0efb045d9b3854f5a4ae838c64e4d11e75005 and referenced by '//tensorboard/components/tf_imports:umap-js'
ERROR: Evaluation of query "deps(//tensorboard)" failed: errors were encountered while computing transitive closure
There was a problem hiding this comment.
Ok, updated the hash and it seems to work from a clean cache now.
Re: testing the integrated features-- the main change is to project new points (umap.transform()), which I manually tested in the chrome dev tools since the embedding projector doesn't use this feature normally.
The one problem now, though, is that umap-js.d.ts in tensorboard doesn't match the new minified file. It looks like the umap declaration file is copied from this file in the umap repo, but the corresponding 1.2.2 version of that file doesn't seem to exist, and neither yarn build nor yarn bundle generated it. Any thoughts? I can also bother Andy about it (it's his TODO :) )
There was a problem hiding this comment.
Hmm, good catch. Looks like this was deleted in this PR:
PAIR-code/umap-js#3
Yes, let’s bother Andy :-). @cannoneyed, was this removal intentional?
It doesn’t look like there are any .d.ts files left in the repository.
What was the intended migration plan?
There was a problem hiding this comment.
So I was chatting with @cannoneyed about this on Friday, and he said that umap-js.d.ts was actually manually created and was just for the specific methods that were used in the embedding projector (Andy, let me know if I'm misunderstanding.)
Given that none of those methods are changing, maybe we don't have to update umap-js.d.ts?
There was a problem hiding this comment.
I think Andy's now out on paternity leave-- @wchargin, do you have thoughts on what to do here?
|
Hey all,
Indeed - I'm on paternity and traveling quite a bit so I'm not able to
comment too deeply on this - basically, I think the best way to move
forward is to continue hackily "solving" the umap typings in the repo by
updating the .d.ts file by hand. There's not really a good workflow for
creating a .d.ts file for a globally (ie via script tag) imported library,
and I experimented with a few build setups that would export a .d.ts file
but they were more trouble than they were worth. So I think it's best to
continue just doing it the way I did it to begin with, unless there's an
easier way to import ts libraries into tensorboard from npm nowadays.
All the best, and looking forward to seeing you two when I get back!
Andy
…On Fri, Nov 22, 2019 at 1:51 PM Emily Reif ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In third_party/js.bzl
<#2898 (comment)>
:
> @@ -212,8 +212,8 @@ def tensorboard_js_workspace():
licenses = ["notice"], # Apache License 2.0
sha256_urls = {
"85a2ff924f1bf4757976aca22fd0efb045d9b3854f5a4ae838c64e4d11e75005": [
I think Andy's now out on paternity leave-- @wchargin
<https://github.com/wchargin>, do you have thoughts on what to do here?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2898?email_source=notifications&email_token=AB3U2WHKKTYNMHL6XI26TOTQVAZ5TA5CNFSM4JJ4KIK2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCMW6DKA#discussion_r349767749>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB3U2WBVQRCXOYNHXCDOLPTQVAZ5TANCNFSM4JJ4KIKQ>
.
|
|
Sounds good, thanks Andy! Hope the Jude-bonding time is going well :) @wchargin-- Re umap: given what Andy's saying, should we go ahead and just submit it as is, since I think the .d.ts file is still up to date for what the embedding projector needs? |
|
Sorry-- didn't mean to close the issue! @wchargin , is there anything else I should do here? Would it be ok to just submit it as is, since the .d.ts file is still up to date for what the embedding projector needs? This is blocking an internal prototype we're working on, so it would be great to get it resolved soon. |
There was a problem hiding this comment.
Hi—sorry for the delay; we’ve been having some CI fires, but those
should mostly be resolved now.
It’s certainly not great to leave the orphaned .d.ts here, especially
given that the public API has changed in this delta: new public method
transform, new public functions initTransform, reshape2d, etc.
If the UMAP build system no longer supports generating a .d.ts file,
could you please update the comment in our .d.ts to reflect that this
is a hand-chosen subset of the API that is sufficient for what we use,
rather than claiming that it’s automatically generated?
|
Oh no! Good luck with the fires... Yeah sure! I updated the comment-- let me know if there's a better way of phrasing it or more info I should add |
Motivation for features / changes
Updating umap version to match internal google version.
Technical description of changes
Just a change to js.bzl
Screenshots of UI changes
n/a
Detailed steps to verify changes work correctly (as executed by you)
Manually tested with bazel run tensorboard/plugins/projector/vz_projector:devserver. All seemed to be in order.
Alternate designs / implementations considered
n/a