-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Make profile plugin use EventMultiplexer to find data in subdirectories #1931
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lesystem call overhead
…ntMultiplexer plugin asset system
I haven't adjusted the tests at all yet, so those will fail for now. I'll fix them in a followup commit but wanted to get this up for review and testing. |
qiuminxu
approved these changes
Mar 1, 2019
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you also need to add an event file to the profile demo.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes #500 by making the profile plugin use EventMultiplexer's plugin assets handling logic to find profile plugin data in arbitrarily deep logdir subdirectories, as long as those subdirectories contain at least one tfevents file. It's an alternative to PR #1360.
The plugin assets system is deprecated, but the profile plugin data already matches the format expected by that system, and we don't have a better way yet to manage large blobs of plugin-specific data that are too big or unwieldy to want to shove into event files.
One complication here is that the profile plugin has a particular complex plugin assets usage pattern in its
plugins/profile
subdirectories. Each such directory can itself contain multiple "runs" in the profile plugin terminology - not to be confused with TensorBoard "runs" which are any subdirectories within the logdir root that containtfevents
files. So a given TensorBoard run may contain many profile plugin runs. This previously wasn't a huge concern because the profile plugin had no real concept of TensorBoard runs, since it only used the logdir root run (aka the.
run). I distinguish these in the new code as "TB/TensorBoard runs" versus "profile runs".To work around this limitation without frontend changes, this borrows the notion from #1360 of including both the TensorBoard run and the profile run in the "run" concept passed around by the profile plugin frontend (and used in URL parameters to query the plugin backend), which the new code calls a "frontend run" or just "run". I tweaked the logic from #1360 so that instead of including the entire directory path, we just include the part below the logdir, and we omit the
plugins/profile
intermediate path components. This generates more concise and user-friendly run names. The long term solution should probably be refactoring the frontend so that it exposes the TensorBoard runs directly in their own selection menu, and then within a given TensorBoard run, exposes the existing drop-down for a "profile run".I've taken some pains to make the
is_active()
implementation fairly efficient (since this blocks overall TensorBoard loading, and was the primary concern with the independent traversal approach from #1360). The maingenerate_run_to_tools()
logic is in a generator form so thatis_active()
can short-circuit and return early once we've found an active run. However, querying the EventMultiplexer for plugin assets still hits every single run directory, which is slow; we should attempt to do this on the main reload background thread and cache it so this lookup can be fast.