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.

Sign up for GitHub

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

Jump to bottom

ENH: Add log text reader function #367

Merged
merged 7 commits into from
May 25, 2022
Merged

ENH: Add log text reader function #367

merged 7 commits into from
May 25, 2022

Conversation

peterhessey
Copy link
Contributor

Adds a util function for reading the text contained in log files. Designed to work for boht std_log.txt (new runtime) and 70_driver_log.txt. Used only once so far in hi-ml but has use in other tasks in InnerEye, such as here.

@peterhessey peterhessey requested review from fepegar and ant0nsc May 20, 2022 10:36
@peterhessey
Copy link
Contributor Author

peterhessey commented May 20, 2022 •
edited
Loading

Also re. unit tests: mocking Run objects properly seems like a daunting task somewhat out of the scope of this issue. Fortunately this function has been tested in end-to-end tests using both runtimes (AML seems to pick at random which it's going to use) and the tests pass. However, the test case where no log file is present is not covered by these tests.

@peterhessey peterhessey marked this pull request as draft May 20, 2022 10:39
@codecov
Copy link

codecov bot commented May 20, 2022 •
edited
Loading

Codecov Report

Merging #367 (8c5892b) into main (47be597) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

Flag Coverage Δ
hi-ml 80.86% <ø> (ø)
hi-ml-azure 81.67% <100.00%> (+0.15%) ⬆️
hi-ml-histopathology 75.46% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
hi-ml-azure/src/health_azure/utils.py 82.92% <100.00%> (+0.26%) ⬆️

@peterhessey peterhessey marked this pull request as ready for review May 20, 2022 10:44
fepegar
fepegar requested changes May 20, 2022
View reviewed changes
ant0nsc
ant0nsc requested changes May 20, 2022
View reviewed changes
Copy link
Collaborator

@ant0nsc ant0nsc left a comment

Choose a reason for hiding this comment

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

We have 2 occurences of 70_driver_log.txt in code, and one in documentation. Could you clean up those as part of this PR?

@peterhessey peterhessey force-pushed the phessey/add-log-file-loader branch 4 times, most recently from 3f71498 to 9acb43a Compare May 20, 2022 14:14
@peterhessey peterhessey requested review from ant0nsc and fepegar May 20, 2022 14:46
fepegar
fepegar requested changes May 20, 2022
View reviewed changes
@peterhessey peterhessey force-pushed the phessey/add-log-file-loader branch from 37d0851 to b4c7a23 Compare May 20, 2022 15:12
@peterhessey peterhessey force-pushed the phessey/add-log-file-loader branch from b4c7a23 to 8c5892b Compare May 25, 2022 07:14
@peterhessey peterhessey merged commit 33a1306 into main May 25, 2022
@peterhessey peterhessey deleted the phessey/add-log-file-loader branch May 25, 2022 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ant0nsc ant0nsc ant0nsc approved these changes

@fepegar fepegar fepegar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@peterhessey @ant0nsc @fepegar