Skip to content
This repository has been archived by the owner on Sep 18, 2024. It is now read-only.

Suport prefix url for nnimanager #3643

Merged
merged 5 commits into from
May 24, 2021

Conversation

acured
Copy link
Contributor

@acured acured commented May 14, 2021

Suport prefix url for nnimanager

@@ -19,7 +20,7 @@ import { createRestHandler } from './restHandler';
*/
@component.Singleton
export class NNIRestServer extends RestServer {
private readonly API_ROOT_URL: string = '/api/v1/nni';
private readonly API_ROOT_URL: string = `/api/v1/nni/${getUrlPrefix()}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think prefix URL should better override /api/v1/nni.

Copy link
Contributor

Choose a reason for hiding this comment

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

The same opinion with @liuzhe-lz , in

API_ROOT_URL = '/api/v1/nni'

so that we don't need to modify too many internal interfaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

@Lijiaoa Lijiaoa mentioned this pull request May 14, 2021
3 tasks
@ultmaster ultmaster linked an issue May 14, 2021 that may be closed by this pull request
@ultmaster ultmaster requested a review from J-shang May 14, 2021 08:38
@@ -81,6 +81,10 @@ def start_rest_server(port, platform, mode, experiment_id, foreground=False, log
cmds += ['--log_level', log_level]
if foreground:
cmds += ['--foreground', 'true']
if url_prefix:
path_validation(url_prefix)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe validate in create_experiment is better?
and if we resume or view an experiment, should we support reconfigure url_prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Changed.
  2. I'm ok for that, since there is no way to change "url_prefix". Any concerns @liuzhe-lz ?

@ultmaster ultmaster merged commit 35c3d16 into microsoft:master May 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is there any option to set base url for the web report?
4 participants