Make `ReportService` sync #660

Swatinem · 2024-08-29T15:15:38Z

It looks like historically there is a mix of sync/async code.
The consensus(?) seems to favor sync code as celery is fundamentally sync.
Async code also does not play as well with profiling, which will not show the async code directly in a flamegraph, instead just inserting a wait for the async code running in a different thread.

codecov-notifications · 2024-08-29T15:22:09Z

Codecov Report

Attention: Patch coverage is 97.75281% with 2 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files	Patch %	Lines
services/report/__init__.py	91.30%	2 Missing ⚠️

@@            Coverage Diff             @@
##             main     #660      +/-   ##
==========================================
- Coverage   98.06%   97.93%   -0.13%     
==========================================
  Files         437      437              
  Lines       36892    36864      -28     
==========================================
- Hits        36179    36104      -75     
- Misses        713      760      +47

Flag	Coverage Δ
integration	`97.93% <97.75%> (-0.13%)`	⬇️
latest-uploader-overall	`97.93% <97.75%> (-0.13%)`	⬇️
unit	`97.93% <97.75%> (-0.13%)`	⬇️

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

Components	Coverage Δ
NonTestCode	`95.62% <92.59%> (-0.32%)`	⬇️
OutsideTasks	`98.03% <97.70%> (-0.03%)`	⬇️

Files	Coverage Δ
services/bundle_analysis/report.py	`100.00% <100.00%> (ø)`
services/notification/notifiers/tests/conftest.py	`100.00% <100.00%> (ø)`
services/test_results.py	`90.10% <100.00%> (ø)`
services/tests/test_report.py	`100.00% <100.00%> (ø)`
tasks/preprocess_upload.py	`98.14% <100.00%> (ø)`
tasks/upload.py	`96.19% <100.00%> (ø)`
services/report/__init__.py	`96.31% <91.30%> (-1.08%)`	⬇️

... and 2 files with indirect coverage changes

codecov-qa · 2024-08-29T15:22:15Z

Codecov Report

Attention: Patch coverage is 97.75281% with 2 lines in your changes missing coverage. Please review.

Project coverage is 97.93%. Comparing base (3cdf975) to head (67e5adc).
Report is 1 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
services/report/__init__.py	91.30%	2 Missing ⚠️

@@            Coverage Diff             @@
##             main     #660      +/-   ##
==========================================
- Coverage   98.06%   97.93%   -0.13%     
==========================================
  Files         437      437              
  Lines       36892    36864      -28     
==========================================
- Hits        36179    36104      -75     
- Misses        713      760      +47

Flag	Coverage Δ
integration	`97.93% <97.75%> (-0.13%)`	⬇️
latest-uploader-overall	`97.93% <97.75%> (-0.13%)`	⬇️
unit	`97.93% <97.75%> (-0.13%)`	⬇️

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

Components	Coverage Δ
NonTestCode	`95.62% <92.59%> (-0.32%)`	⬇️
OutsideTasks	`98.03% <97.70%> (-0.03%)`	⬇️

Files with missing lines	Coverage Δ
services/bundle_analysis/report.py	`100.00% <100.00%> (ø)`
services/notification/notifiers/tests/conftest.py	`100.00% <100.00%> (ø)`
services/test_results.py	`90.10% <100.00%> (ø)`
services/tests/test_report.py	`100.00% <100.00%> (ø)`
tasks/preprocess_upload.py	`98.14% <100.00%> (ø)`
tasks/upload.py	`96.19% <100.00%> (ø)`
services/report/__init__.py	`96.31% <91.30%> (-1.08%)`	⬇️

... and 2 files with indirect coverage changes

codecov-public-qa · 2024-08-29T15:22:30Z

Codecov Report

Attention: Patch coverage is 97.75281% with 2 lines in your changes missing coverage. Please review.

Project coverage is 97.93%. Comparing base (3cdf975) to head (67e5adc).

✅ All tests successful. No failed tests found.

@@            Coverage Diff             @@
##             main     #660      +/-   ##
==========================================
- Coverage   98.06%   97.93%   -0.13%     
==========================================
  Files         437      437              
  Lines       36892    36864      -28     
==========================================
- Hits        36179    36104      -75     
- Misses        713      760      +47

Flag	Coverage Δ
integration	`97.93% <97.75%> (-0.13%)`	⬇️
latest-uploader-overall	`97.93% <97.75%> (-0.13%)`	⬇️
unit	`97.93% <97.75%> (-0.13%)`	⬇️

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

Components	Coverage Δ
NonTestCode	`95.62% <92.59%> (-0.32%)`	⬇️
OutsideTasks	`98.03% <97.70%> (-0.03%)`	⬇️

Files	Coverage Δ
services/bundle_analysis/report.py	`100.00% <100.00%> (ø)`
services/notification/notifiers/tests/conftest.py	`100.00% <100.00%> (ø)`
services/test_results.py	`90.10% <100.00%> (ø)`
services/tests/test_report.py	`100.00% <100.00%> (ø)`
tasks/preprocess_upload.py	`98.14% <100.00%> (ø)`
tasks/upload.py	`96.19% <100.00%> (ø)`
services/report/__init__.py	`96.31% <91.30%> (-1.08%)`	⬇️

... and 2 files with indirect coverage changes

codecov · 2024-08-29T15:22:47Z

Codecov Report

Attention: Patch coverage is 97.75281% with 2 lines in your changes missing coverage. Please review.

Project coverage is 97.98%. Comparing base (3cdf975) to head (67e5adc).
Report is 1 commits behind head on main.

Changes have been made to critical files, which contain lines commonly executed in production. Learn more

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
services/report/__init__.py Critical	91.30%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #660      +/-   ##
==========================================
- Coverage   98.11%   97.98%   -0.13%     
==========================================
  Files         476      476              
  Lines       38213    38185      -28     
==========================================
- Hits        37492    37417      -75     
- Misses        721      768      +47

Flag	Coverage Δ
integration	`97.93% <97.75%> (-0.13%)`	⬇️
latest-uploader-overall	`97.93% <97.75%> (-0.13%)`	⬇️
unit	`97.93% <97.75%> (-0.13%)`	⬇️

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

Components	Coverage Δ
NonTestCode	`95.74% <92.59%> (-0.31%)`	⬇️
OutsideTasks	`98.03% <97.70%> (-0.03%)`	⬇️

Files with missing lines	Coverage Δ
services/bundle_analysis/report.py	`100.00% <100.00%> (ø)`
services/notification/notifiers/tests/conftest.py	`100.00% <100.00%> (ø)`
services/test_results.py	`90.10% <100.00%> (ø)`
services/tests/test_report.py	`100.00% <100.00%> (ø)`
tasks/preprocess_upload.py	`98.18% <100.00%> (ø)`
tasks/upload.py Critical	`97.59% <100.00%> (ø)`
services/report/__init__.py Critical	`96.32% <91.30%> (-1.08%)`	⬇️

... and 2 files with indirect coverage changes

Related Entrypoints
run/app.tasks.upload.Upload
run/app.tasks.notify.Notify
run/app.tasks.timeseries.save_commit_measurements
run/app.tasks.pulls.Sync
run/app.tasks.compute_comparison.ComputeComparison
run/app.tasks.upload.UploadFinisher
run/app.tasks.upload.UploadProcessor
run/app.tasks.upload.PreProcessUpload
run/app.tasks.label_analysis.process_request

matt-codecov

nice, thank you for the cleanup! you are right that sync is preferred. i got rid of a lot of the asyncs in #304 in a semi-automated fashion but didn't get em all

everything used to be labeled async, but the django ORM (which we have begun slowly converging on, away from sqlalchemy) hates that and as you observed all our work is synchronous so we never had anything to yield to

It looks like historically there is a mix of sync/async code. The consensus(?) seems to favor sync code as celery is fundamentally sync. Async code also does not play as well with profiling, which will not show the async code directly in a flamegraph, instead just inserting a `wait` for the async code running in a different thread.

Swatinem requested a review from a team August 29, 2024 15:15

Swatinem self-assigned this Aug 29, 2024

Swatinem changed the title ~~Make ReportService.initialize_and_save_report sync~~ Make ReportService sync Aug 29, 2024

Swatinem force-pushed the swatinem/sync-initreport branch from d8b1b1d to 18985e0 Compare August 29, 2024 15:39

matt-codecov approved these changes Aug 30, 2024

View reviewed changes

Swatinem force-pushed the swatinem/sync-initreport branch from 18985e0 to 67e5adc Compare August 30, 2024 08:36

Swatinem added this pull request to the merge queue Aug 30, 2024

Merged via the queue into main with commit 26de857 Aug 30, 2024
13 of 26 checks passed

Swatinem deleted the swatinem/sync-initreport branch August 30, 2024 09:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `ReportService` sync #660

Make `ReportService` sync #660

Swatinem commented Aug 29, 2024

codecov-notifications bot commented Aug 29, 2024 •

edited

Loading

codecov-qa bot commented Aug 29, 2024 •

edited

Loading

codecov-public-qa bot commented Aug 29, 2024 •

edited

Loading

codecov bot commented Aug 29, 2024 •

edited

Loading

matt-codecov left a comment •

edited

Loading

Make ReportService sync #660

Make ReportService sync #660

Conversation

Swatinem commented Aug 29, 2024

codecov-notifications bot commented Aug 29, 2024 • edited Loading

Codecov Report

codecov-qa bot commented Aug 29, 2024 • edited Loading

Codecov Report

codecov-public-qa bot commented Aug 29, 2024 • edited Loading

Codecov Report

codecov bot commented Aug 29, 2024 • edited Loading

Codecov Report

matt-codecov left a comment • edited Loading

Choose a reason for hiding this comment

Make `ReportService` sync #660

Make `ReportService` sync #660

codecov-notifications bot commented Aug 29, 2024 •

edited

Loading

codecov-qa bot commented Aug 29, 2024 •

edited

Loading

codecov-public-qa bot commented Aug 29, 2024 •

edited

Loading

codecov bot commented Aug 29, 2024 •

edited

Loading

matt-codecov left a comment •

edited

Loading