Skip to content
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

DL4J UI: Add multi-session support #7185

Merged
merged 8 commits into from Mar 13, 2019

Conversation

@printomi
Copy link

commented Feb 18, 2019

What changes were proposed in this pull request?

This closes #7110
Add optional multi-session functionality to UIServer: separate sessions by including session ID in URL-s, like /train/:sessionId/overview instead of /train/overview in single-session UI-server.

  • Add --multiSession command-line option to PlayUIServer. Pass this setting is to TrainModule and DefaultModule. The front-end javascript code queries this setting to choose working mode.
  • enabling multi-session mode: change PlayUIServer constructor and add command-line option (--multiSession), add UIServer factory method
  • logging attached and detached sessions with URL for convenience in multi-session mode
  • JavaScript: in multi-session mode, the session ID is determined from the URL. A new function getSessionSettings(callback) defined in train.js is used to get this information, then query URLs for single/multi-session mode are determined in the callback.
  • Set separate i18n for sessions in multi-session mode.
  • add option for attaching StatsStorage based on session ID passed in the URL, via custom provider, when in multi-session mode

This closes #7274
Fix missing dependency for UIServer.stop(), handle stopped state, add test.

How was this patch tested?

New manual tests in TestPlayUIMultiSession to check multi-session support, auto-attaching functionality and an auto-attaching-detaching example.
New test in TestPlayUI for stopping functionality.

Quick checklist

The following checklist helps ensure your PR is complete:

  • Reviewed the Contributing Guidelines and followed the steps within.
  • Created tests for any significant new code additions.
  • Relevant tests for your changes are passing.
  • Ran mvn formatter:format (see formatter instructions for targeting your specific files).
@printomi

This comment has been minimized.

Copy link
Author

commented Feb 18, 2019

I can imagine a unit test for the auto-attach functionality (PlayUIServer.autoAttachStatsStorageBySessionId(Function<String, StatsStorage> statsStorageProvider)). With this, I could even create a stress-test, to see, whether we should use more synchronization in TrainModule. Shall I try it?

@printomi printomi force-pushed the printomi:ui_multisession branch 2 times, most recently from f95a9a9 to e857a6f Feb 18, 2019

@printomi

This comment has been minimized.

Copy link
Author

commented Feb 18, 2019

With the improved tests, I stressed the UI-server with hundreds of parallel training sessions, without any error. I think, that the current synchronization works well enough, so I don't think that method-level synchronization is needed in TrainModule.

@printomi printomi changed the title [WIP] DL4J UI: Add multi-session support DL4J UI: Add multi-session support Feb 18, 2019

@printomi printomi force-pushed the printomi:ui_multisession branch from e857a6f to 5d4181c Feb 20, 2019

@printomi

This comment has been minimized.

Copy link
Author

commented Feb 20, 2019

I have reviewed the changes, and made some refactoring and fixes. @AlexDBlack what do you think about this PR?

@printomi

This comment has been minimized.

Copy link
Author

commented Feb 22, 2019

For the sake of completeness, I suggest, multi-session mode should be enabled in ConvolutionalListenerModule, too. In multi-session mode, one would access this page at /activations/:sessionId, instead of /activations. Shall I do it?

@AlexDBlack
Copy link
Contributor

left a comment

Apologies for not reviewing this sooner.

Overall, this is pretty good. I don't think I have any major issues with the design or implementation. 👍
I noted a few relatively minor issues. Otherwise, I think this is close to being mergeable (I'll need to build and test it manually as a final check though).

@AlexDBlack

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

For the sake of completeness, I suggest, multi-session mode should be enabled in ConvolutionalListenerModule, too. In multi-session mode, one would access this page at /activations/:sessionId, instead of /activations. Shall I do it?

ConvolutionalListener needs some other fixes, as you already know: #7217
Perhaps we tackle it separately, and fix all 3 at once? (I was looking at trying to fix that some time this week, though it isn't a very high priority item for me...)

@printomi

This comment has been minimized.

Copy link
Author

commented Feb 25, 2019

I agree, let's tackle ConvolutionalListener issues separately, outside of this PR.

@AlexDBlack
Copy link
Contributor

left a comment

So, testing this: it all works and looks good. 👍

One possible usability improvement: When user goes to http://localhost:9000/ they get this:

UI server is in multi-session mode. You can find a training session at http://localhost:9000/train/:sessionId (See console for attached training session ID.)

Could we simply list the sessions here, and provide links to training?

Anyway, let's add that single/multi session check + exception (and list sessions if you want) and I'm happy to merge this.

@printomi

This comment has been minimized.

Copy link
Author

commented Feb 28, 2019

I agree that listing the available sessions on the main page would improve usability in multi-session mode.
I am ready with the list, you can try it.

Also, I finished the single/multi session check + exception.

@printomi printomi force-pushed the printomi:ui_multisession branch from eedd56b to 439cc9f Feb 28, 2019

@printomi

This comment has been minimized.

Copy link
Author

commented Feb 28, 2019

I have just made a rebase to reduce commit count, after the last commit 439cc9f.

@printomi

This comment has been minimized.

Copy link
Author

commented Feb 28, 2019

One question remained for me: how can I stop UIServer without getting the above-mentioned exception?

@AlexDBlack
Copy link
Contributor

left a comment

@printomi Sorry, looks like I introduced a conflict for you in the UI scala templates, in my recent PR here: https://github.com/deeplearning4j/deeplearning4j/pull/7245/files
It was a simple path fix in one of the files, but I had to regenerate all scala template files.
You should be able to rebase on master (doesn't matter which .template.scala files you keep) then regenerate them with the buildUiTemplates profile.

Otherwise this LGTM 👍
Happy to merge this after the conflict has been resolved.

printomi added 8 commits Dec 6, 2018
Add optional multi-session functionality to UIServer: separate sessio…
…ns by including session ID in URL-s, like /train/:sessionId/overview instead of /train/overview in single-session UI-server.

- Add --multiSession command-line option to PlayUIServer. Pass this setting is to TrainModule and DefaultModule. The front-end javascript code queries this setting to choose working mode.
- enabling multi-session mode: change PlayUIServer constructor and add command-line option (--multiSession), add UIServer factory method
- logging attached and detached sessions with URL for convenience in multi-session mode
- JavaScript: in multi-session mode, the session ID is determined from the URL. A new function getSessionSettings(callback) defined in train.js is used to get this information, then query URLs for single/multi-session mode are determined in the callback.
- Set separate i18n for sessions in multi-session mode.
- add option for attaching StatsStorage based on session ID passed in the URL, via custom provider, when in multi-session mode

- fixes in TrainModule
    - handle null value of lastUpdateForSession and currentSessionID in TrainModule
    - detaching: clean up lastUpdateForSession in
    - some typos
    - Make some functions static, that were private in TrainModule, and do not use any fields of the instance.
    - handle i18n no data condition
    - Fill `lastUpdateForSession` in `onAttach`, too. Without this, if a `StatsStorage` is attached (containing updates) that does not receive any updates, the `lastUpdateForSession` would not contain its update timestamp.

- Remove routes that are not used anywhere in the code:
    - /train/sessions/all (in `TrainModule`)
    - /train/workers/currentByIdx (in `TrainModule`)
    - /lang/getCurrent (in `PlayUIServer`)
- page templates
    - main page (brand) link fix
    - Remove Ukrainian ("uk") option from the language selector.
    - regenerate scala page templates
Improved tests and fixes for UI
Fixes:
- only log attached and detached sessions with URL in multi-session mode
- prevent PlayUIServer from propagating event of a StatsStorage that is not attached yet.
- check for null value of a collection before foreach in TrainModule.getMemory(...)
Refactor TrainModule, check null values, undo change
Undo unneccessary change in a previous commit: don't return 404 at /train/sessions/info and /train/system/data if there is no data

@printomi printomi force-pushed the printomi:ui_multisession branch from dad7bb9 to f680d8f Mar 12, 2019

@printomi

This comment has been minimized.

Copy link
Author

commented Mar 12, 2019

@AlexDBlack I have rebased onto master, and regenerated scala templates.

@AlexDBlack
Copy link
Contributor

left a comment

LGTM - Thanks again for the PR! 👍

@AlexDBlack AlexDBlack merged commit 1d996a2 into eclipse:master Mar 13, 2019

0 of 3 checks passed

codeclimate Code Climate encountered an error attempting to analyze this pull request.
Details
continuous-integration/jenkins/pr-head This commit cannot be built
Details
Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details

@printomi printomi deleted the printomi:ui_multisession branch Mar 13, 2019

@printomi

This comment has been minimized.

Copy link
Author

commented Mar 13, 2019

Thanks a lot, @AlexDBlack . I can now start to build upon this, using snapshots.

@printomi

This comment has been minimized.

Copy link
Author

commented Apr 29, 2019

For the sake of completeness, I suggest, multi-session mode should be enabled in ConvolutionalListenerModule, too. In multi-session mode, one would access this page at /activations/:sessionId, instead of /activations. Shall I do it?

ConvolutionalListener needs some other fixes, as you already know: #7217
Perhaps we tackle it separately, and fix all 3 at once? (I was looking at trying to fix that some time this week, though it isn't a very high priority item for me...)

Now that issue #7217 has been closed, and I can confirm that TestConvolutionalListener.testUI() passes, I would ask again, if multi-session support for ConvolutionalListenerModule is welcome in DL4J-UI?
If so, I would file an issue about this, and I might have time to implement it in the next few weeks.
(It is not a high priority for me, but for the sake of completeness, I think it would worth.)

@AlexDBlack

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

@printomi Sure, if you want to, go ahead and do that. I don't think it's something we would want to allocate internal resources to though (other than reviewing PRs etc). Feel free to open an issue about it too if you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.