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

[Retiarii] Visualization #3878

Merged
merged 15 commits into from
Jul 12, 2021
Merged

Conversation

ultmaster
Copy link
Contributor

@ultmaster ultmaster commented Jun 25, 2021

This is the first working example of Retiarii visualization.

TODOs:

  • Hide visualization button for non-Retiarii mode.
  • Review current limitation that forces users to update browser settings. (fixed using reverse proxy)
  • Add documentation.

@ultmaster ultmaster marked this pull request as draft June 25, 2021 08:09
@ultmaster ultmaster marked this pull request as ready for review June 28, 2021 04:34
ts/webui/src/static/style/openRow.scss Outdated Show resolved Hide resolved
ts/webui/src/static/model/experiment.ts Outdated Show resolved Hide resolved
updated ||= !compareProfiles(this.profileField, profile);
this.profileField = profile;

updated ||= JSON.stringify(this.metadataField) !== JSON.stringify(metadata);
Copy link
Contributor

Choose a reason for hiding this comment

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

need to update page when metadata update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure. Removing it for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I could test it during the bug bash.

@@ -8,7 +8,7 @@
*/
type TrialJobStatus = 'UNKNOWN' | 'WAITING' | 'RUNNING' | 'SUCCEEDED' | 'FAILED' | 'USER_CANCELED' | 'SYS_CANCELED' | 'EARLY_STOPPED';

type LogType = 'TRIAL_LOG' | 'TRIAL_STDOUT' | 'TRIAL_ERROR';
type LogType = 'TRIAL_LOG' | 'TRIAL_STDOUT' | 'TRIAL_ERROR' | 'MODEL.onnx';
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove it if the parameter type is 'string' or keep this type for parameter and rename it as "FileName"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just removed LogType as I think it's no longer used.

@@ -12,6 +12,8 @@ import { getLogDir } from '../common/utils';
import { createRestHandler } from './restHandler';
import { getAPIRootUrl } from '../common/experimentStartupInfo';

const httpProxy = require('http-proxy');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not import? type definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

idk. Copied code. I'll try import.

if (log === '') {
log = 'No logs available.'
private getTrialFile(router: Router): void {
router.get('/trial-file/:id/:filename', async(req: Request, res: Response) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

seems ut in restful_server.py use /trial-log/:id/:type

if (content instanceof Buffer) {
res.header('Content-Type', 'application/octet-stream');
} else if (content === '') {
content = 'No logs available.'
Copy link
Contributor

Choose a reason for hiding this comment

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

No logs available. seems strange since it is getTrialFile() function.

throw new Error(`File unaccessible: ${fileName}`);
}
let encoding: string | null = null;
if (!fileName.includes('.') || fileName.match(/.*\.(txt|log)/g)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest rename stdout to stdout.txt...
Not the responsibility of this pr though...

updated ||= !compareProfiles(this.profileField, profile);
this.profileField = profile;

if (JSON.stringify(this.metadataField) !== JSON.stringify(metadata)) {
Copy link
Contributor

@liuzhe-lz liuzhe-lz Jul 9, 2021

Choose a reason for hiding this comment

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

Seems this if is meaningless

@@ -59,7 +59,7 @@ abstract class Manager {
public abstract getMetricDataByRange(minSeqId: number, maxSeqId: number): Promise<MetricDataRecord[]>;
public abstract getLatestMetricData(): Promise<MetricDataRecord[]>;

public abstract getTrialLog(trialJobId: string, logType: LogType): Promise<string>;
public abstract getTrialFile(trialJobId: string, fileName: string): Promise<Buffer | string>;
Copy link
Contributor

Choose a reason for hiding this comment

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

The behavior would be more predictable to split into get text and get binary.
If you have time, not mandatory...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm having a hard time thinking of another meaningful name.
Let's do it in future when we actually need this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is a broken change

@QuanluZhang QuanluZhang merged commit 1bd17a0 into microsoft:master Jul 12, 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.

7 participants