Logging client #1017

franchuterivera · 2020-11-26T21:32:49Z

Moving to log client exclusively and removed other support
Adding checker to make sure expected messages are in log file

codecov · 2020-11-26T22:01:32Z

Codecov Report

Merging #1017 (d2b317c) into development (7ef86da) will increase coverage by 0.15%.
The diff coverage is 95.83%.

@@               Coverage Diff               @@
##           development    #1017      +/-   ##
===============================================
+ Coverage        85.36%   85.51%   +0.15%     
===============================================
  Files              125      127       +2     
  Lines             9891    10145     +254     
===============================================
+ Hits              8443     8676     +233     
- Misses            1448     1469      +21

Impacted Files	Coverage Δ
autosklearn/evaluation/test_evaluator.py	`90.90% <ø> (ø)`
autosklearn/evaluation/train_evaluator.py	`72.74% <ø> (ø)`
autosklearn/metalearning/mismbo.py	`100.00% <ø> (ø)`
autosklearn/metalearning/metalearning/meta_base.py	`87.83% <50.00%> (ø)`
autosklearn/util/logging_.py	`89.68% <81.81%> (-4.63%)`	⬇️
autosklearn/smbo.py	`82.53% <85.71%> (+0.24%)`	⬆️
autosklearn/evaluation/abstract_evaluator.py	`88.44% <87.50%> (+0.01%)`	⬆️
autosklearn/util/backend.py	`76.15% <94.44%> (+1.98%)`	⬆️
autosklearn/automl.py	`85.02% <100.00%> (+0.14%)`	⬆️
autosklearn/ensemble_builder.py	`76.56% <100.00%> (ø)`
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ef86da...d2b317c. Read the comment docs.

mfeurer

First batch of review, but only this simple stuff so far...

autosklearn/metalearning/metafeatures/metafeatures.py

autosklearn/metalearning/metafeatures/metafeature.py

autosklearn/metalearning/metalearning/kNearestDatasets/kND.py

autosklearn/metalearning/metalearning/meta_base.py

autosklearn/metalearning/optimizers/metalearn_optimizer/metalearner.py

autosklearn/evaluation/abstract_evaluator.py

.pre-commit-config.yaml

test/test_evaluation/test_train_evaluator.py

test/test_evaluation/test_test_evaluator.py

test/test_evaluation/test_evaluation.py

mfeurer

Part 2.

autosklearn/evaluation/__init__.py

autosklearn/util/backend.py

test/test_automl/automl_utils.py

mfeurer

Review 3/3.

Did you check if this works in singularity? Could you maybe add a test the log files are not created in the current working directory in case the logging is set up correctly?

test/test_automl/automl_utils.py

autosklearn/util/logging_.py

mfeurer · 2020-11-30T08:41:06Z

Thanks for the updates @franchuterivera the changes look good, but I'll wait with approving until all tests are green.

When looking at the tests I realized that there's a lot of output now and I'm not sure where it comes from, but it makes finding the failed tests quite hard. Do you know where it comes from and whether it can be reduced again?

franchuterivera · 2020-11-30T11:34:48Z

Sorry, I also missed to add the sparse file and there was a bug here and there.

I have fixed the output of the pytest.

I have added a new test to see what things are left behind by pytest to catch if we will be able to run singularity. This is likely going to fail if we miss some file. Once I see it I will fix this.

scripts/run_auto-sklearn_for_metadata_generation.py

mfeurer

This now looks good to me. However, there are still several files under autosklearn.metalearning which have logger=None in the constructor. Could you please make this a mandatory argument?

mfeurer · 2020-12-15T15:12:09Z

test/test_automl/test_automl.py

+        # Make sure that running jobs are properly tracked. Killed runs do not always
+        # print the return value to the log file (yet the run history has this information)
+        if run_value.status == StatusType.SUCCESS:
+            # Success is not sufficient to guarantee a return message in the log file


Just checking, if the run doesn't print r'pynisher]\s+return value:\s+' it should print either or "Your function call closed the pipe prematurely -> Subprocess probably got an uncatchable signal." or "Something else went wrong, sorry." in the controlling process, so we could actually check that, right?

If a failure happens here we will get a double count message, because while sending the pipe message it could break.

However, we can make this deterministic and add a message at the end of the pynisher function call (inside pynisher)? For instance here

But aren't the two error messages I mentioned exactly the error messages showing up in case of a failure to write to the pipe?

mfeurer · 2020-12-15T15:14:20Z

autosklearn/evaluation/__init__.py

@@ -261,8 +266,15 @@ def run(
        if self.init_params is not None:
            init_params.update(self.init_params)

+        if self.port is None:


The handling of port is None is different for this logger and the logger above. Can this be unified?

The only reason this is not unified is to get a different name for the TAE and pynisher call. Is it ok if we only get TAE in the message name?

Sorry, I meant that in one case it check if the port is None, in the other it doesn't check.

franchuterivera · 2020-12-16T19:58:50Z

I have created #1039 to make testing more restrictive after the running task left out at the end of smac is resolved

mfeurer reviewed Nov 27, 2020

View reviewed changes

franchuterivera force-pushed the logging_client branch from 9f87fe3 to a406274 Compare November 30, 2020 11:38

mfeurer reviewed Nov 30, 2020

View reviewed changes

scripts/run_auto-sklearn_for_metadata_generation.py Outdated Show resolved Hide resolved

mfeurer reviewed Nov 30, 2020

View reviewed changes

franchuterivera force-pushed the logging_client branch from 9b2ca34 to 3ba5604 Compare December 3, 2020 18:59

TAE first

5ba9e6e

mfeurer mentioned this pull request Dec 7, 2020

AutoSklearnClassifier seems to exit application on finishing fit #1029

Closed

franchuterivera added 6 commits December 8, 2020 13:15

wa-18.04

38e54f1

Fix unit testing

c3ab578

Now metalearning logger

fe72694

Final touches to logging

08c09ae

Fix flake

6f38022

Mypy fix

9d9762d

franchuterivera force-pushed the logging_client branch from 63c45b9 to 9d9762d Compare December 8, 2020 19:19

franchuterivera marked this pull request as draft December 8, 2020 19:19

franchuterivera added 5 commits December 8, 2020 23:17

Clean up unit testing

17627e8

Missed ensemble builder!

e1f7f40

Fix unit test

1f2f3a2

Consider failed runs with success status

f89f5b8

Make testlimit consistent

fb61376

franchuterivera requested a review from mfeurer December 11, 2020 13:26

franchuterivera marked this pull request as ready for review December 14, 2020 16:04

mfeurer reviewed Dec 15, 2020

View reviewed changes

franchuterivera added 2 commits December 16, 2020 20:16

Feedback from comments

beab69e

Mising space

d2b317c

mfeurer merged commit 4c6bef0 into automl:development Dec 17, 2020

github-actions bot pushed a commit that referenced this pull request Dec 17, 2020

Francisco Rivera Valverde: Logging client (#1017)

3459dde

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Logging client #1017

Logging client #1017

franchuterivera commented Nov 26, 2020

codecov bot commented Nov 26, 2020 •

edited

mfeurer left a comment

mfeurer left a comment

mfeurer left a comment

mfeurer commented Nov 30, 2020

franchuterivera commented Nov 30, 2020

mfeurer left a comment

mfeurer Dec 15, 2020

franchuterivera Dec 15, 2020

mfeurer Dec 16, 2020

mfeurer Dec 15, 2020

franchuterivera Dec 15, 2020

mfeurer Dec 16, 2020

franchuterivera commented Dec 16, 2020

Logging client #1017

Logging client #1017

Conversation

franchuterivera commented Nov 26, 2020

codecov bot commented Nov 26, 2020 • edited

Codecov Report

mfeurer left a comment

Choose a reason for hiding this comment

mfeurer left a comment

Choose a reason for hiding this comment

mfeurer left a comment

Choose a reason for hiding this comment

mfeurer commented Nov 30, 2020

franchuterivera commented Nov 30, 2020

mfeurer left a comment

Choose a reason for hiding this comment

mfeurer Dec 15, 2020

Choose a reason for hiding this comment

franchuterivera Dec 15, 2020

Choose a reason for hiding this comment

mfeurer Dec 16, 2020

Choose a reason for hiding this comment

mfeurer Dec 15, 2020

Choose a reason for hiding this comment

franchuterivera Dec 15, 2020

Choose a reason for hiding this comment

mfeurer Dec 16, 2020

Choose a reason for hiding this comment

franchuterivera commented Dec 16, 2020

codecov bot commented Nov 26, 2020 •

edited