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

Speed up unit tests by removing sleep in start/stop torchserve #2383

Merged
merged 5 commits into from
Jun 5, 2023

Conversation

mreso
Copy link
Collaborator

@mreso mreso commented Jun 1, 2023

Description

This PR removes sleep(10) in our unit test utils and replaces it with a function which parses the log to wait the actual time it takes to start torchserve. This speeds up single unit tests by up to 4x.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Feature/Issue validation/testing

Please describe the Unit or Integration tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.

  • Run pytest test/pytest/test_example_micro_batching.py before and after change

Before:
Screenshot 2023-06-01 at 1 39 44 PM

After:
Screenshot 2023-06-01 at 1 40 29 PM

Checklist:

  • Did you have fun?

@codecov
Copy link

codecov bot commented Jun 1, 2023

Codecov Report

Merging #2383 (0d8281e) into master (e205e6b) will decrease coverage by 0.10%.
The diff coverage is 0.00%.

❗ Current head 0d8281e differs from pull request most recent head 24651ea. Consider uploading reports for the commit 24651ea to get more accurate results

@@            Coverage Diff             @@
##           master    #2383      +/-   ##
==========================================
- Coverage   72.13%   72.03%   -0.10%     
==========================================
  Files          78       78              
  Lines        3642     3647       +5     
  Branches       58       58              
==========================================
  Hits         2627     2627              
- Misses       1011     1016       +5     
  Partials        4        4              
Impacted Files Coverage Δ
ts/arg_parser.py 25.80% <ø> (ø)
ts/model_server.py 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mreso mreso force-pushed the better_engineering/remove_sleep branch from ce08534 to 79be2c2 Compare June 1, 2023 23:33
@mreso mreso marked this pull request as ready for review June 2, 2023 00:51
@mreso mreso changed the title [WIP] Speed up unit tests by removing sleep in start/stop torchserve Speed up unit tests by removing sleep in start/stop torchserve Jun 2, 2023
@mreso mreso force-pushed the better_engineering/remove_sleep branch from 79be2c2 to 568fb1f Compare June 2, 2023 03:11
}


class LogPipeTillTheEnd(threading.Thread):
Copy link
Member

@msaroufim msaroufim Jun 2, 2023

Choose a reason for hiding this comment

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

Is the assumption that each new run of torchserve will overwite its logs?

It seems safe since tests are passing but just a bit worried we'll run into hard to debug testing issues later down the line

Copy link
Collaborator Author

@mreso mreso Jun 2, 2023

Choose a reason for hiding this comment

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

The tests still run sequentially and nothing is overwritten. This class only prints the logs even after TS initialization is completed. This replicates the same behavior as we had before where we ran TS in a background process and waited for 10 seconds. Now we parse the stdout pipe for the initialization completed signal and then just let this class print the rest of the log. This way folks can still see and debug issues as before.
EDIT: Wrong class, this class actually writes a log but the principle is the same. Previously we just piped the output into a file, now we first parse it and then write it into the file. All other behavior stays the same. From a dev perspective nothing changes besides that your unit test is not waiting 30+ seconds for nothing.
EDIT2: Okay, three time's a charm...the log file gets appended every times, so same behavior as ">>" vs ">". We could probably get the same result with a tee -a if you prefer that.

@msaroufim msaroufim self-requested a review June 5, 2023 16:39
@mreso mreso merged commit 28a2525 into master Jun 5, 2023
@mreso mreso deleted the better_engineering/remove_sleep branch June 5, 2023 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants