[batch] Use job group id in front end and driver queries #13995

jigold · 2023-11-09T16:52:47Z

Stacked on #13985. This PR just threads job_group_id everywhere such as the canceller, scheduler, etc.

jigold · 2023-11-30T20:38:57Z

This is ready for review. Remember it's stacked on the last database PR.

daniel-goldstein

This is great! These changes feel very routine, I just had a handful of quite small questions and suggestions.

daniel-goldstein · 2023-11-30T21:33:52Z

batch/batch/driver/instance_collection/job_private.py

-FROM batches
-INNER JOIN jobs ON batches.id = jobs.batch_id
+FROM job_groups
+LEFT JOIN batches ON batches.id = job_groups.batch_id


Nit: This is guaranteed to find a matching row in batches right? Any reason it's a left join not an INNER JOIN?

I usually try and use left joins rather than inner joins because the errors will be obvious if a row is not present when we expect it to be (NULL values will cause downstream errors). With an inner join, we could silently skip rows. But we do have all of the foreign key constraints so you're right. Let me know if you have a preference. Mine is not particularly strong

I usually try and use left joins rather than inner joins because the errors will be obvious if a row is not present when we expect it to be (NULL values will cause downstream errors)

This sounds very reasonable! I certainly errors and alerts to silent failures, can just keep this as is

daniel-goldstein · 2023-11-30T21:44:24Z

batch/batch/driver/instance_collection/pool.py

-                if not batch['cancelled']:
-                    async for record in self.db.select_and_fetchall(
+                if not job_group['cancelled']:
+                    async for record in self.db.select_and_fetchall(  # FIXME: Do we need a different index?


Is there an answer to this question? If not, it might be better to instead make a github issue that requires an investigation into this query plan and can be closed by confirming that no additional index is needed or by providing one.

Good catch. I think we do need to add a new index and modify the last ORDER BY clause to use the job group. Let's add that in #13985.

daniel-goldstein · 2023-11-30T21:45:12Z

batch/batch/driver/job.py

@@ -39,26 +40,27 @@ async def notify_batch_job_complete(db: Database, client_session: httpx.ClientSe
  job_groups_n_jobs_in_complete_states.n_succeeded,
  job_groups_n_jobs_in_complete_states.n_failed,
  job_groups_n_jobs_in_complete_states.n_cancelled
-FROM batches
+FROM job_groups
+LEFT JOIN batches ON job_groups.batch_id = batches.id


same question here on the left vs inner join.

as above, let's just keep as is

daniel-goldstein · 2023-11-30T21:48:19Z

batch/batch/driver/job.py

@@ -436,6 +439,7 @@ async def job_config(app, record, attempt_id):
    return {
        'batch_id': batch_id,
        'job_id': job_id,
+        'job_group_id': job_group_id,


How come the job_group_id is both in this dict and in the job_spec field?

I don't think it's in the job_spec field in this PR. I intended to pop that value from the job spec regardless as it could be relative in #14018. Maybe this change should come later once we want the job_group_id to be available as an environment variable on the worker.

I'm going to remove this functionality for now from this PR as I have a different PR later on that puts the right job group id in the spec when creating jobs.

daniel-goldstein · 2023-11-30T21:49:21Z

batch/batch/driver/job.py

@@ -446,14 +450,15 @@ async def job_config(app, record, attempt_id):
    }


-async def mark_job_errored(app, batch_id, job_id, attempt_id, user, format_version, error_msg):
+async def mark_job_errored(app, batch_id, job_id, attempt_id, job_group_id, user, format_version, error_msg):


nit: I think it's more consistent and therefore less error-prone to order these inputs batch_id -> job_group_id -> job_id -> attempt_id

daniel-goldstein · 2023-11-30T21:50:50Z

batch/batch/driver/job.py

    format_version = BatchFormatVersion(record['format_version'])

    id = (batch_id, job_id)

    try:
-        body = await job_config(app, record, attempt_id)
+        body = await job_config(app, record, attempt_id, job_group_id)


Since job_group_id is already in record, I don't think we need to add it as an extra input.

daniel-goldstein · 2023-11-30T21:53:36Z

batch/batch/driver/main.py

-    LEFT JOIN job_groups_cancelled ON batches.id = job_groups_cancelled.id
-    WHERE batches.`state` = 'running'
+    FROM jobs
+    INNER JOIN job_groups ON job_groups.batch_id = jobs.batch_id AND job_groups.job_group_id = jobs.job_group_id


How come you swapped the order of the tables in the join? (batches join jobs vs jobs join job_groups). I'm not sure it makes any difference because it's an inner join but it stuck out to me.

No reason other than it could have been a rebase oversight as the conflicts were a bit gnarly at one point. What makes the most sense to you?

I have a mild preference for not making changes that don't really change anything, so let's just make this FROM job_groups

daniel-goldstein · 2023-11-30T21:54:49Z

batch/batch/driver/main.py

  JSON_OBJECTAGG(resources.resource, quantity * GREATEST(COALESCE(rollup_time - start_time, 0), 0)) as resources
 FROM attempt_resources
 INNER JOIN attempts
 ON attempts.batch_id = attempt_resources.batch_id AND
  attempts.job_id = attempt_resources.job_id AND
  attempts.attempt_id = attempt_resources.attempt_id
+LEFT JOIN jobs ON attempts.batch_id = jobs.batch_id AND attempts.job_id = jobs.job_id


similar question here as before, I would assume this would be an inner join. Shouldn't affect correctness but I don't know what the idiomatic thing to do here is.

As above, we'll keep as is

daniel-goldstein · 2023-11-30T22:14:03Z

batch/batch/front_end/front_end.py

 ''',
-        (user, batch_id, update_id),
+        (user, batch_id, ROOT_JOB_GROUP_ID, update_id),


Just for my understanding: will this always remain ROOT_JOB_GROUP_ID because we only allow updates if the whole batch, i.e. the root job group, is not cancelled?

daniel-goldstein · 2023-11-30T22:19:53Z

batch/test/test_invariants.py

@@ -23,5 +23,5 @@ async def test_invariants():

        data = await retry_transient_errors(session.get_read_json, url, headers=headers)

-        assert data['check_incremental_error'] is None, data
-        assert data['check_resource_aggregation_error'] is None, data
+        assert data['check_incremental_error'] == 'None', data


How come this changed to strings?

Because an actual exception is not JSON serializable making this functionality useless for figuring out what went wrong.

Ah, gotcha, nice catch! In that case, it's a little pedantic but I'd prefer to make more use of the type system and have an Optional[str] where no exception does show up as None and if there is an exception we serialize that as a string. The string 'None' feels off to me.

done

daniel-goldstein

a couple small changes left in the comments

done

…ob-group-server

jigold · 2024-02-01T19:45:06Z

Closing in favor of #14170

jigold force-pushed the thread-job-group-server branch from c7a014a to 250a1f6 Compare November 9, 2023 19:50

jigold marked this pull request as ready for review November 13, 2023 19:56

jigold force-pushed the thread-job-group-server branch from 9266bf5 to 48a3233 Compare November 14, 2023 20:20

jigold assigned daniel-goldstein Nov 14, 2023

jigold added stacked PR WIP migration labels Nov 14, 2023

jigold force-pushed the thread-job-group-server branch from 48a3233 to 339931d Compare November 14, 2023 20:28

jigold mentioned this pull request Nov 14, 2023

[batch] Use job group infrastructure for batches where applicable #14011

Closed

jigold removed the migration label Nov 14, 2023

jigold mentioned this pull request Nov 16, 2023

[batch] Add job groups in client and list/get job group jobs on server #14016

Closed

jigold force-pushed the thread-job-group-server branch from 18f0c7a to 1b37ed1 Compare November 29, 2023 17:33

jigold added 7 commits November 30, 2023 12:28

[batch] Finalize job groups in database

80b664a

fix

0b50b03

fix for ambig column

6c9c776

fix foreign key constraint

284b457

dont lock primary key updates

d1fd11a

fix cancel job group

8ac5425

last fix?

5ffa578

jigold force-pushed the thread-job-group-server branch from b858d07 to 5749bc6 Compare November 30, 2023 17:38

jigold added 2 commits November 30, 2023 13:07

get rid of extra complexity

03cdaa5

fixup estimated-current.sql

904a045

jigold force-pushed the thread-job-group-server branch from 5749bc6 to b16b283 Compare November 30, 2023 18:25

fix cancel child job groups

3bdca11

daniel-goldstein previously requested changes Nov 30, 2023

View reviewed changes

add new index

e7fe638

daniel-goldstein previously requested changes Dec 1, 2023

View reviewed changes

jigold added 6 commits January 12, 2024 10:58

add back batch updates fields

e09c512

[batch] Use job group id in front end and driver queries

b40bff2

address comments

1e20595

get rid of exposing job group id to worker

ed95628

address comments

e6ed1f0

delint

853d949

jigold force-pushed the thread-job-group-server branch from 7e514e2 to 853d949 Compare January 12, 2024 16:13

jigold added 4 commits January 12, 2024 12:56

Merge commit '24525adb9c09a73a1ae820c9945acd35299878ed' into thread-j…

4c2b750

…ob-group-server

Merge commit 'fa2ef0f2c76654d0c037ff6db60ccb8842fb8539' into thread-j…

d7d3b53

…ob-group-server

partial ruff apply

1dc4ce9

partial ruff apply

b777802

jigold closed this Feb 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[batch] Use job group id in front end and driver queries #13995

[batch] Use job group id in front end and driver queries #13995

jigold commented Nov 9, 2023 •

edited

Loading

jigold commented Nov 30, 2023

daniel-goldstein left a comment

daniel-goldstein Nov 30, 2023

jigold Dec 1, 2023

daniel-goldstein Dec 1, 2023

daniel-goldstein Nov 30, 2023

jigold Dec 1, 2023

daniel-goldstein Nov 30, 2023

jigold Dec 1, 2023

daniel-goldstein Dec 1, 2023

daniel-goldstein Nov 30, 2023

jigold Dec 1, 2023

jigold Dec 1, 2023

daniel-goldstein Nov 30, 2023

daniel-goldstein Nov 30, 2023

daniel-goldstein Nov 30, 2023

jigold Dec 1, 2023

daniel-goldstein Dec 1, 2023

daniel-goldstein Nov 30, 2023

jigold Dec 1, 2023

daniel-goldstein Dec 1, 2023

daniel-goldstein Nov 30, 2023

jigold Dec 1, 2023

daniel-goldstein Nov 30, 2023

jigold Nov 30, 2023

daniel-goldstein Dec 1, 2023

daniel-goldstein left a comment

jigold commented Feb 1, 2024

[batch] Use job group id in front end and driver queries #13995

[batch] Use job group id in front end and driver queries #13995

Conversation

jigold commented Nov 9, 2023 • edited Loading

jigold commented Nov 30, 2023

daniel-goldstein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-goldstein left a comment

Choose a reason for hiding this comment

jigold commented Feb 1, 2024

jigold commented Nov 9, 2023 •

edited

Loading