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

Draft modification to redirect logs to test output #4710

Merged
merged 10 commits into from
Feb 1, 2020
Merged

Draft modification to redirect logs to test output #4710

merged 10 commits into from
Feb 1, 2020

Conversation

harishsk
Copy link
Contributor

@harishsk harishsk commented Jan 26, 2020

Currently when running tests the channel output doesn't appear in the test logs. This is a draft change to enable that functionality in order to get better logging to debug tests better.

There are two changes here.
The first is a simple adds a handler for MLContext's Log event and logs the message to test output. The second adds a TextWriter derived class to ConsoleEnvironment that makes ConsoleEnvironment also log to the test console if the code is running a test environment.

I am putting this PR up for review for two purposes.

The first is for general verification of the code and concept. And while the code reviews are in progress I am also using this to try and debug the failure in BinaryClassifierSymSgdTest in the CI builds.

@harishsk harishsk requested a review from a team as a code owner January 26, 2020 01:00
@harishsk harishsk requested a review from a team as a code owner January 28, 2020 05:21
@justinormont
Copy link
Contributor

Related issues on having an output verbosity level besides zero & firehose:

@harishsk
Copy link
Contributor Author

I understand the issue about the all or nothing logs. It would be good to add additional params to control the verbosity but that will impact the public API. I want the full logs right now to debug test failures. Can we defer the issue of adding additional control params?

@justinormont
Copy link
Contributor

Can we defer the issue of adding additional control params?

Oh certainly. I was hoping/dreaming that both the unit test and user level logging could be solved in the same way.

@harishsk
Copy link
Contributor Author

I have added additional params to LoggingEventArgs

@sharwell
Copy link
Member

Can we not log to a file that gets attached as a build artifact?

@harishsk
Copy link
Contributor Author

I think it is better in the default test log. It gives more flexibility that if I am running a single test locally on the console, I can see the output on the console or redirect to a file as I see fit. And on the build server, it gets logged to a file anyway. It it still a file whether it is in the artifacts or the default log file.

Copy link
Contributor

@frank-dong-ms-zz frank-dong-ms-zz left a comment

Choose a reason for hiding this comment

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

:shipit:

@harishsk harishsk merged commit 609003e into dotnet:master Feb 1, 2020
@harishsk harishsk deleted the testLogger branch April 21, 2020 23:59
@ghost ghost locked as resolved and limited conversation to collaborators Mar 19, 2022
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