Skip to content

[DebuggerV2] Add basic store support and component for graph execution #3506

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.

Sign up for GitHub

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

Jump to bottom
Merged
caisq merged 4 commits into from
Apr 15, 2020
Merged

[DebuggerV2] Add basic store support and component for graph execution #3506

caisq merged 4 commits into from
Apr 15, 2020

Conversation

@caisq
Copy link
Contributor

@caisq caisq commented Apr 12, 2020 •
edited
Loading

  • Motivation for features / changes
    • Continue developing DebuggerV2 plugin, specifically the part that focuses on intra-graph execution.
  • Technical description of changes
    • debugger_types: add graphExecutions field to DebuggerState
    • tfdbg2_data_source.ts: add fetchGraphExecutionDigests() and fetchGraphExecutionData()
    • debugger_actions.ts: add action for requesting # of graph executions and action that signifies the successful loading of the # of graph executions
    • debugger_reducers.ts: add reducers for the said actions and states
    • views/graph_executions folder: add component and container for visualizing graph executions. Currently only the number of graph executions is shown. Follow-up PRs will add a virtual scroll element that shows the graph execution digests.
    • Update the CSS and layout in the top-section of DebuggerV2 Plugin to accommodate the new GraphExecutionContainer.
  • Screenshots of UI changes
    • image
  • Detailed steps to verify changes work correctly (as executed by you)
    • Unit tests added
    • Running the DebuggerV2 plugin against a logdir with real tfdbg2 dump data

@caisq caisq marked this pull request as ready for review April 12, 2020 19:08
@caisq caisq requested a review from stephanwlee April 13, 2020 12:56
stephanwlee
stephanwlee approved these changes Apr 15, 2020
View reviewed changes
Copy link
Contributor

@stephanwlee stephanwlee left a comment

Choose a reason for hiding this comment

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

Can you tell us why we need to make a separate RPC for just one number? Can we fetch all metadata about debugging information with one endpoint instead? Does that cause our database implementations to scan more than we'd like?

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store/debugger_types.ts Outdated
/**
* Base interface shared between top-level and intra-graph executions.
*
* Supports paged lazy loading of digess (i.e., concise data objects
Copy link
Contributor

@stephanwlee stephanwlee Apr 15, 2020

Choose a reason for hiding this comment

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

s/digess/digest

Supports paged lazy loading of digest doesn't read nicely properly in my head. It may be just me.

Copy link
Contributor Author

@caisq caisq Apr 15, 2020

Choose a reason for hiding this comment

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

Fixed the typo and tweaked the wording here for clarification.

caisq
caisq commented Apr 15, 2020
View reviewed changes
Copy link
Contributor Author

@caisq caisq left a comment

Choose a reason for hiding this comment

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

Thanks for the review!

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/store/debugger_types.ts Outdated
/**
* Base interface shared between top-level and intra-graph executions.
*
* Supports paged lazy loading of digess (i.e., concise data objects
Copy link
Contributor Author

@caisq caisq Apr 15, 2020

Choose a reason for hiding this comment

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

Fixed the typo and tweaked the wording here for clarification.

@caisq
Copy link
Contributor Author

caisq commented Apr 15, 2020

@stephanwlee about the question of why separate RPCs for single numbers. Yes, there are three two such rpcs currently:

  • /executions/digests for the number of top-level (eager) executions
  • /graph_execution/digests for the number of intra-graph executions.

Theoretically, they can be grouped into a single rpc call. But the argument for not combining them include:

  1. It's nice to have a conceptual separation between the two. They do use different tags at the DataProvider level.
  2. There won't be more of them. It's not like there is a third or fourth type of execution. So there won't be too much concern about a proliferation of such things in the future.

@caisq caisq merged commit 76a24c8 into tensorflow:master Apr 15, 2020
@caisq caisq deleted the dbg2-graph-exec-1a branch April 15, 2020 21:55
caisq added a commit to caisq/tensorboard that referenced this pull request May 19, 2020
@caisq
[DebuggerV2] Add basic store support and component for graph execution (
2fba10b 
tensorflow#3506)

* Motivation for features / changes
  * Continue developing DebuggerV2 plugin, specifically the part that focuses on intra-graph execution.
* Technical description of changes
  * debugger_types: add `graphExecutions` field to `DebuggerState`
  * tfdbg2_data_source.ts: add `fetchGraphExecutionDigests()` and `fetchGraphExecutionData()`
  * debugger_actions.ts: add action for requesting # of graph executions and action that signifies the successful loading of the # of graph executions
  * debugger_reducers.ts: add reducers for the said actions and states
  * views/graph_executions folder: add component and container for visualizing graph executions. Currently only the number of graph executions is shown. Follow-up PRs will add a virtual scroll element that shows the graph execution digests.
  * Update the CSS and layout in the top-section of DebuggerV2 Plugin to accommodate the new `GraphExecutionContainer`.
* Screenshots of UI changes
  * ![image](https://user-images.githubusercontent.com/16824702/79076659-ecb86680-7cc9-11ea-895b-2a4fa6089ad2.png)
* Detailed steps to verify changes work correctly (as executed by you)
  * Unit tests added
  * Running the DebuggerV2 plugin against a logdir with real tfdbg2 dump data
caisq added a commit that referenced this pull request May 27, 2020
@caisq
[DebuggerV2] Add basic store support and component for graph execution (
f4c7679 
#3506)

* Motivation for features / changes
  * Continue developing DebuggerV2 plugin, specifically the part that focuses on intra-graph execution.
* Technical description of changes
  * debugger_types: add `graphExecutions` field to `DebuggerState`
  * tfdbg2_data_source.ts: add `fetchGraphExecutionDigests()` and `fetchGraphExecutionData()`
  * debugger_actions.ts: add action for requesting # of graph executions and action that signifies the successful loading of the # of graph executions
  * debugger_reducers.ts: add reducers for the said actions and states
  * views/graph_executions folder: add component and container for visualizing graph executions. Currently only the number of graph executions is shown. Follow-up PRs will add a virtual scroll element that shows the graph execution digests.
  * Update the CSS and layout in the top-section of DebuggerV2 Plugin to accommodate the new `GraphExecutionContainer`.
* Screenshots of UI changes
  * ![image](https://user-images.githubusercontent.com/16824702/79076659-ecb86680-7cc9-11ea-895b-2a4fa6089ad2.png)
* Detailed steps to verify changes work correctly (as executed by you)
  * Unit tests added
  * Running the DebuggerV2 plugin against a logdir with real tfdbg2 dump data
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stephanwlee stephanwlee stephanwlee approved these changes

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@caisq @stephanwlee @googlebot