Remove functionality of legacy DB mode #3539
Merged
Conversation
This file contains hidden or 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
Summary: TensorBoard typically reads from a logdir on disk, but the `--db_import` and `--db` flags offered an experimental SQLite-backed read path. This DB mode has always been clearly marked as experimental, and does not work as advertised. For example, runs were broken in all dashboards. A data set with *n* runs would show as *n* runs in TensorBoard, but all of them would have the name `.` and the same color, and only one of them would actually render data. The images dashboard was also completely broken and did not show images. And lots of plugins just didn’t support DB mode at all. Furthermore, the existence of DB mode is an active impediment to maintenance, and also accounts for some of the few remaining test cases that require TensorFlow 1.x to run. This patch removes support for DB mode from all plugins, removes the `db_connection_provider` field from `TBContext`, and renders the `--db` and `--db_import` flags functionally inoperative. The plumbing itself will be removed in a separate commit; a modicum of care is required for the `TensorBoardInfo` struct. Test Plan: Running with a standard demo logdir, TensorBoard still works fine with both `--generic_data false` and `--generic_data true`. wchargin-branch: nodb-functionality
wchargin
commented
Apr 22, 2020
| "No histogram tag %r for run %r" % (tag, run) | ||
| ) | ||
| (tag_id,) = row | ||
| # Fetch tensor values, optionally with linear-spaced sampling by step. |
There was a problem hiding this comment.
RIP cool linear sampling SQL query, 2018–2020.
nfelt
approved these changes
Apr 23, 2020
There was a problem hiding this comment.
Thanks for doing this! 💯✖️🥝
DB mode has always been clearly marked as experimental, and does not
work as advertised. For example, runs were broken in all dashboards.
A data set with n runs would show as n runs in TensorBoard, but all
of them would have the name.and the same color, and only one of them
would actually render data.
For posterity, not to defend this from a UX standpoint, but I think this was because the DB mode "logic" was to treat each top-level subdirectory of the logdir as an "experiment", in which runs were separately namespaced, which is why all the runs show up as .. Basically to make it look right you had to use --logdir_spec to pass in something like exp1:/tmp/exp1,exp2:/tmp/exp2.
| "No histogram tag %r for run %r" % (tag, run) | ||
| ) | ||
| (tag_id,) = row | ||
| # Fetch tensor values, optionally with linear-spaced sampling by step. |
tensorboard/plugins/base_plugin.py
Outdated
| function is cheap. The returned connection must only be used by a | ||
| single thread. Things like connection pooling are considered | ||
| implementation details of the provider. | ||
| db_uri: The string db URI TensorBoard was started with. If this is set, |
There was a problem hiding this comment.
Should db_uri also be removed here, or is it being used in the manager code somehow?
There was a problem hiding this comment.
Yep; thanks. Done.
wchargin-branch: nodb-functionality wchargin-source: 8df9359e969a0291beb459fa2ed49a97a3050f47
wchargin
commented
Apr 23, 2020
tensorboard/plugins/base_plugin.py
Outdated
| function is cheap. The returned connection must only be used by a | ||
| single thread. Things like connection pooling are considered | ||
| implementation details of the provider. | ||
| db_uri: The string db URI TensorBoard was started with. If this is set, |
There was a problem hiding this comment.
Yep; thanks. Done.
15 tasks
caisq
pushed a commit
to caisq/tensorboard
that referenced
this pull request
May 19, 2020
Summary: TensorBoard typically reads from a logdir on disk, but the `--db_import` and `--db` flags offered an experimental SQLite-backed read path. This DB mode has always been clearly marked as experimental, and does not work as advertised. For example, runs were broken in all dashboards. A data set with *n* runs at top level would show as *n* runs in TensorBoard, but all of them would have the name `.` and the same color, and only one of them would actually render data. The images dashboard was also completely broken and did not show images. And lots of plugins just didn’t support DB mode at all. Furthermore, the existence of DB mode is an active impediment to maintenance, and also accounts for some of the few remaining test cases that require TensorFlow 1.x to run. This patch removes support for DB mode from all plugins, removes the `db_connection_provider` field from `TBContext`, and renders the `--db` and `--db_import` flags functionally inoperative. The plumbing itself will be removed in a separate commit; a modicum of care is required for the `TensorBoardInfo` struct. Test Plan: Running with a standard demo logdir, TensorBoard still works fine with both `--generic_data false` and `--generic_data true`. wchargin-branch: nodb-functionality
9 tasks
caisq
pushed a commit
that referenced
this pull request
May 27, 2020
Summary: TensorBoard typically reads from a logdir on disk, but the `--db_import` and `--db` flags offered an experimental SQLite-backed read path. This DB mode has always been clearly marked as experimental, and does not work as advertised. For example, runs were broken in all dashboards. A data set with *n* runs at top level would show as *n* runs in TensorBoard, but all of them would have the name `.` and the same color, and only one of them would actually render data. The images dashboard was also completely broken and did not show images. And lots of plugins just didn’t support DB mode at all. Furthermore, the existence of DB mode is an active impediment to maintenance, and also accounts for some of the few remaining test cases that require TensorFlow 1.x to run. This patch removes support for DB mode from all plugins, removes the `db_connection_provider` field from `TBContext`, and renders the `--db` and `--db_import` flags functionally inoperative. The plumbing itself will be removed in a separate commit; a modicum of care is required for the `TensorBoardInfo` struct. Test Plan: Running with a standard demo logdir, TensorBoard still works fine with both `--generic_data false` and `--generic_data true`. wchargin-branch: nodb-functionality
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary:
TensorBoard typically reads from a logdir on disk, but the
--db_importand
--dbflags offered an experimental SQLite-backed read path. ThisDB mode has always been clearly marked as experimental, and does not
work as advertised. For example, runs were broken in all dashboards.
A data set with n runs at top level would show as n runs in
TensorBoard, but all of them would have the name
.and the same color,and only one of them would actually render data. The images dashboard
was also completely broken and did not show images. And lots of plugins
just didn’t support DB mode at all. Furthermore, the existence of DB
mode is an active impediment to maintenance, and also accounts for some
of the few remaining test cases that require TensorFlow 1.x to run.
This patch removes support for DB mode from all plugins, removes the
db_connection_providerfield fromTBContext, and renders the--dband
--db_importflags functionally inoperative. The plumbing itselfwill be removed in a separate commit; a modicum of care is required for
the
TensorBoardInfostruct.Test Plan:
Running with a standard demo logdir, TensorBoard still works fine with
both
--generic_data falseand--generic_data true.wchargin-branch: nodb-functionality