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

chore: bunify db/postgres_tasks.go #8764

Merged
merged 5 commits into from
Feb 1, 2024

Conversation

carolinaecalderon
Copy link
Contributor

@carolinaecalderon carolinaecalderon commented Jan 26, 2024

Description

Bunify methods in db/postgres_tasks.go, and add a context.Context parameter to those functions.

For functions that call methods from postgres_tasks.go, define ctx := context.Background() if it's not already passed in as a parameter. For functions that passed in the db as a parameter, but no longer need it because the relevant method is bun-ified, remove the db parameter. (in other words, streamline the substitutions).

Remove unused functions in postgres_test_utils.go, postgres.go.

For the method completeTrialsTask, originally in postgres_tasks.go, remove the function wrapper, and insert it directly into the postgres_trials.go method where it's only used once.

errors.Errorf(...) --> fmt.Errorf(...) for all methods bun-ified, or majorly refactored.

Unmocked the db in telemetry_test.go since there was a newly bunified db tasks call there -- followed the same template as postgres_tasks_intg_test.go & started the db in TestMain.

Test Plan

See postgres_tasks_intg_test.go, originally from #8750, but edited here to include context.Context parameters etc

Commentary (optional)

To limit the scope of this ticket:

  1. I didn't bunify any of the model.TaskLogs functions. They're tricky (use sql filters, or use queryProto for super long queries, or have better perf with bulk-insert than Bun), and part of the TaskLogBackend interface. Honestly, I think this deserves its own ticket if we prioritize bun-ifying it. Untangling the db struct from the TaskLogBackend interface also isn't super obvious.
  2. I didn't move db/postgres_tasks.go to the task module. The task db functions are called in several places within the db module still, so if we attempt to move it out before all these other calls are in separate modules, then we'll get a very messy import cycle. I propose bunifying all files in db first (or at least the job/cluster/checkpoint/experiment/trial) and then moving to their respective module. I'm happy to create another ticket or JIRA epic on the order of bunifying the db module & moving them if the team cares -- but (1) is this a high enough priority and (2) will this be a cluster/resource management/?? responsibility?

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

DET-7957

@cla-bot cla-bot bot added the cla-signed label Jan 26, 2024
@carolinaecalderon carolinaecalderon changed the title Bunify task pt2 convert to bun chore: bunify postgres_tasks.go Jan 26, 2024
Copy link

codecov bot commented Jan 26, 2024

Codecov Report

Attention: 32 lines in your changes are missing coverage. Please review.

Comparison is base (ec850ae) 47.68% compared to head (91e46dc) 47.69%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #8764    +/-   ##
========================================
  Coverage   47.68%   47.69%            
========================================
  Files        1049     1049            
  Lines      167345   167230   -115     
  Branches     2241     2239     -2     
========================================
- Hits        79804    79762    -42     
+ Misses      87383    87310    -73     
  Partials      158      158            
Flag Coverage Δ
backend 43.09% <73.55%> (+0.02%) ⬆️
harness 64.32% <ø> (ø)
web 42.54% <ø> (ø)

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

Files Coverage Δ
master/internal/checkpoint_gc.go 65.51% <100.00%> (ø)
master/internal/db/postgres.go 53.21% <ø> (+2.69%) ⬆️
master/internal/db/postgres_test_utils.go 91.69% <100.00%> (+14.25%) ⬆️
master/internal/task/allocation.go 76.39% <100.00%> (+0.36%) ⬆️
master/internal/telemetry/reports.go 69.76% <100.00%> (ø)
master/internal/api_experiment.go 54.37% <0.00%> (ø)
master/internal/db/setup.go 48.00% <0.00%> (ø)
master/internal/populate_metrics.go 0.00% <0.00%> (ø)
master/internal/command/command.go 67.82% <0.00%> (ø)
master/internal/rm/agentrm/agent.go 0.00% <0.00%> (ø)
... and 4 more

... and 5 files with indirect coverage changes

@carolinaecalderon carolinaecalderon force-pushed the bunify-task-pt2-convert-to-bun branch 2 times, most recently from 42c737d to fac7217 Compare January 29, 2024 19:00
@carolinaecalderon carolinaecalderon changed the title chore: bunify postgres_tasks.go chore: bunify db/postgres_tasks.go Jan 29, 2024
@carolinaecalderon carolinaecalderon force-pushed the bunify-task-tests branch 3 times, most recently from 32092d7 to d1bb042 Compare January 29, 2024 19:34
@carolinaecalderon carolinaecalderon self-assigned this Jan 29, 2024
@carolinaecalderon carolinaecalderon marked this pull request as ready for review January 30, 2024 15:30
@carolinaecalderon carolinaecalderon requested a review from a team as a code owner January 30, 2024 15:30
@carolinaecalderon carolinaecalderon requested review from eecsliu, NicholasBlaskey and stoksc and removed request for a team January 30, 2024 15:30
@NicholasBlaskey
Copy link
Contributor

I'm unassigning myself since I don't plan on reviewing this since @stoksc left a comment

If you want me to review let me know

@NicholasBlaskey NicholasBlaskey removed their request for review January 31, 2024 14:39
Copy link
Contributor

@stoksc stoksc left a comment

Choose a reason for hiding this comment

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

lgtm, modulo some minor comments

master/internal/db/postgres_trial.go Outdated Show resolved Hide resolved
master/internal/trial.go Outdated Show resolved Hide resolved
master/internal/telemetry/reports.go Outdated Show resolved Hide resolved
master/internal/task/allocation.go Show resolved Hide resolved
master/internal/db/postgres_trial.go Outdated Show resolved Hide resolved
master/internal/db/postgres_tasks.go Outdated Show resolved Hide resolved
master/internal/db/postgres_tasks.go Outdated Show resolved Hide resolved
Base automatically changed from bunify-task-tests to main February 1, 2024 00:51
Copy link

netlify bot commented Feb 1, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit 91e46dc
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/65bbb9e9527bc40007e9c95d

@carolinaecalderon carolinaecalderon merged commit 36a2e29 into main Feb 1, 2024
78 of 83 checks passed
@carolinaecalderon carolinaecalderon deleted the bunify-task-pt2-convert-to-bun branch February 1, 2024 15:50
maxrussell pushed a commit that referenced this pull request Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants