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
More Informative N Jobs Print #511
Conversation
Codecov Report
@@ Coverage Diff @@
## master #511 +/- ##
==========================================
+ Coverage 96.08% 96.09% +0.01%
==========================================
Files 108 109 +1
Lines 8867 8897 +30
==========================================
+ Hits 8520 8550 +30
Misses 347 347
Continue to review full report at Codecov.
|
…retools into update_n_jobs
@@ -44,7 +48,17 @@ def int_es(): | |||
return make_ecommerce_entityset(with_integer_time_index=True) | |||
|
|||
|
|||
def test_scatter_warning(): | |||
with warnings.catch_warnings(record=True) as w: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pytest.warns
could probably work here
|
||
|
||
def test_create_client_and_cluster(): | ||
with warnings.catch_warnings(record=True) as w: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as with test_scatter_warning
featuretools/tests/computational_backend/test_calculate_feature_matrix.py
Show resolved
Hide resolved
warning_string += " Not enough cpu cores({}).".format(cpu_workers) | ||
|
||
if num_tasks < n_jobs: | ||
warning_string += " Not enough tasks({}).".format(num_tasks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this to " Not enough chunks (X), consider reducing the chunk size"
scatter_string = "EntitySet scattered to workers in {:.3f} seconds" | ||
print(scatter_string.format(scatter_time)) | ||
|
||
scatter_string = "EntitySet scattered to {} workers in {:.3f} seconds" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue wanted to remove the decimal part of the "in X seconds" message. Maybe round to integer and if it took less than 1 second display "<1" or "under 1"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added in the rounding but I decided to not add in the "<1" so that I don't have to deal with a complex code cov situation. In theory I could create a context manager to time it, and then run an on complete function which then prints out a special string but it feels too complex for a very minor piece of functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be easiest to just round up to the near integer seconds. This is just diagnostic info, so +/- 1 second doesn't matter that much
scatter_string = "EntitySet scattered to workers in {:.3f} seconds" | ||
print(scatter_string.format(scatter_time)) | ||
|
||
scatter_time = round(end - start) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of rounding up round
rounds to the nearest integer. To avoid getting zero we could use a function that rounds up or take the min of the rounded number and 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the motivation for wanting to tell the user it took 1 second if it actually took 0.001 seconds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought saying something took 0 seconds would seem strange to the user since clearly it took at least some time to do, but it's probably fine so let's just leave it as is.
Also gives a warning if the user asks for more jobs than were created. For example if the user asks for 10 jobs and only 4 workers are created (either due to not enough cores or not enough tasks) a warning will show
Also gives a warning if the EntitySet is not scattered to all of the created workers.