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

Add foreground mode in nnictl #1956

Merged
merged 35 commits into from
Feb 7, 2020

Conversation

SparkSnail
Copy link
Contributor

No description provided.

SparkSnail and others added 30 commits August 6, 2019 18:58
Filter prune algo implementation (microsoft#1655)
Support monitor mode when creating or resuming a new experiment (microsoft#1933)
Add test for documentation build (microsoft#1924)
@SparkSnail SparkSnail requested a review from chicm-ms January 14, 2020 08:58
@SparkSnail SparkSnail requested a review from liuzhe-lz January 14, 2020 12:32
@liuzhe-lz
Copy link
Contributor

I thought this could be done fully in nnictl side.

@SparkSnail
Copy link
Contributor Author

I thought this could be done fully in nnictl side.

The log content of node process is written to log file, if it is operated in nnictl side fully, nnictl need to watch the log change and read the changed lines, which is not a easy way.

if args.foreground:
try:
while True:
log_content = rest_process.stdout.readline().strip().decode('utf-8')
Copy link
Contributor

Choose a reason for hiding this comment

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

how about atderr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no stderr content, since we only use console.log, while stderr catch the content of console.error().

@@ -120,9 +122,15 @@ def start_rest_server(port, platform, mode, config_file_name, experiment_id=None
stderr_file.write(log_header)
if sys.platform == 'win32':
from subprocess import CREATE_NEW_PROCESS_GROUP
process = Popen(cmds, cwd=entry_dir, stdout=stdout_file, stderr=stderr_file, creationflags=CREATE_NEW_PROCESS_GROUP)
if args.foreground:
process = Popen(cmds, cwd=entry_dir, stdout=PIPE, stderr=STDOUT, creationflags=CREATE_NEW_PROCESS_GROUP)
Copy link
Contributor

Choose a reason for hiding this comment

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

a little strange here, for win32, stderr is STDOUT, for linux, stderr is PIPE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://stackoverflow.com/questions/10406532/python-subprocess-output-on-windows
If I set stderr=PIPE in windows, the process will crash, so I redirect stderr=STDOUT.

@@ -81,6 +82,7 @@ class Logger {
}

this.readonly = isReadonly();
this.foreground = foreground;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest to do a little bit refactor on log.ts:
currently fileName being undefined means to use nnimanager.log as output file. change it to:

  1. undefined means send output to STDOUT.
  2. Move file name nnimanager.log into main.ts

Then we do not need a foreground flag for log.ts. Because both log files and STDOUT are places where log class can send content to, we do not need an extract flag foreground for STDOUT from log class perspective.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@SparkSnail SparkSnail merged commit d7920fd into microsoft:master Feb 7, 2020
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.

4 participants