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

test(artifacts): replace sleeps with flush when waiting on a file to write #4523

Merged
merged 3 commits into from Nov 23, 2022

Conversation

moredatarequired
Copy link
Contributor

Description

Replace flaky calls to time.sleep(0.1) with calls to flush when waiting on a file to write.

For some reason on my machine the calls to time.sleep often do nothing. Regardless, this is a faster and more reliable way to ensure files are written with sequential c/m/a times.

Testing

[test only PR, no behavior change]

Checklist

  • Include reference to internal ticket "Fixes WB-NNNN" and/or GitHub issue "Fixes #NNNN" (if applicable)
  • Ensure PR title compliance with the conventional commits standards

For some reason on my machine the calls to  don't work consistently;
this is faster and more reliable as a way to ensure files are written with
sequential c/m/a times.
@codecov
Copy link

codecov bot commented Nov 23, 2022

Codecov Report

Merging #4523 (e7c5aea) into main (d911344) will increase coverage by 0.03%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4523      +/-   ##
==========================================
+ Coverage   83.08%   83.12%   +0.03%     
==========================================
  Files         259      259              
  Lines       32979    32979              
==========================================
+ Hits        27401    27414      +13     
+ Misses       5578     5565      -13     
Flag Coverage Δ
functest 56.95% <ø> (+0.21%) ⬆️
unittest 73.30% <ø> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
wandb/sdk/internal/internal_api.py 89.17% <0.00%> (-0.52%) ⬇️
wandb/sdk/wandb_setup.py 88.44% <0.00%> (-0.51%) ⬇️
wandb/sdk/lib/git.py 79.01% <0.00%> (ø)
wandb/sdk/wandb_run.py 90.72% <0.00%> (+0.05%) ⬆️
wandb/sdk/wandb_settings.py 94.73% <0.00%> (+0.27%) ⬆️
wandb/sdk/interface/artifacts.py 79.03% <0.00%> (+0.34%) ⬆️
wandb/sdk/lib/printer.py 89.24% <0.00%> (+0.63%) ⬆️
wandb/sdk/internal/file_stream.py 89.86% <0.00%> (+1.01%) ⬆️
wandb/sdk/lib/sock_client.py 94.17% <0.00%> (+1.05%) ⬆️
wandb/sdk/internal/system/system_info.py 91.55% <0.00%> (+3.24%) ⬆️
... and 1 more

Copy link
Contributor

@kptkin kptkin left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for the fix!
getting away from time based tests is def an improvement :)

Copy link
Contributor

@vwrj vwrj left a comment

Choose a reason for hiding this comment

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

lgtm!

Nit: from browsing online, looks like the os.fsync method guarantees writes to actual disk, I'm seeing a few folks recommend os.flush() followed by the fysnc.

@moredatarequired moredatarequired enabled auto-merge (squash) November 23, 2022 17:10
@moredatarequired
Copy link
Contributor Author

lgtm!

Nit: from browsing online, looks like the os.fsync method guarantees writes to actual disk, I'm seeing a few folks recommend os.flush() followed by the fysnc.

Ha, I mentioned that in the first PR where I had to add it. I suppose I can be rigorous here instead of stopping once I have it working.

@moredatarequired moredatarequired merged commit f6695ba into main Nov 23, 2022
@moredatarequired moredatarequired deleted the artifacts/flush-test branch November 23, 2022 17:53
andrewtruong pushed a commit that referenced this pull request Dec 2, 2022
…tes (#4523)

test(artifacts): flush writes instead of sleeping

For some reason on my machine the calls to  don't work consistently;
this is faster and more reliable as a way to ensure files are written with
sequential c/m/a times.
@kptkin kptkin changed the title test(artifacts): flush writes instead of sleeping to wait on file writes test(artifacts): replace sleeps with flush when waiting on a file to write Dec 6, 2022
@github-actions github-actions bot added cc-test and removed cc-test labels Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants