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

feat: Generic Tasks #8724

Merged
merged 90 commits into from
Feb 12, 2024
Merged

feat: Generic Tasks #8724

merged 90 commits into from
Feb 12, 2024

Conversation

AmanuelAaron
Copy link
Contributor

@AmanuelAaron AmanuelAaron commented Jan 22, 2024

Description

Creating Generic Tasks API, part of Project Modular Execution. Includes expansion of task CLI to include the following commands:

  • create <config file> [--context CONTEXT] [-i INCLUDE] [--project_id PROJECT_ID] [--config CONFIG] [-f] [--fork FORK] [-p PARENT] [--inherit_context]
  • config <task id> [--json ]
  • fork <parent task id> [--project_id PROJECT_ID]
  • kill <task id> [--root]
  • pause <task id>
  • unpause <task id>

Test Plan

Create Task

  1. Create a config yaml with an entrypoint to some executable code
    Ex:
# test_config.yaml
entrypoint: ["python3", "example.py"]
  1. Create task in cli pointing to the config yaml
$ det task create <path to config> --context <config context>
>> created task <task-id>

Get Config

  1. Get config:
det task config <task-id>

Fork Task

  1. Create Task:

$ det task create e2e_tests/tests/fixtures/generic_task/test_config.yaml --context e2e_tests/tests/fixtures/generic_task
> created task <task-id>

  1. Fork Task:

$ det task create e2e_tests/.../test_config.yaml --context e2e_tests/.../generic_task e2e_tests --fork <task-id>
> created task <forked-task-id>

  1. Check Logs to see if changes in task exist:

$ det task logs <forked-task-id>

...
[<date-time>] <log-id> || forked
...

Kill Task

  1. Create Task:

$ det task create e2e_tests/tests/fixtures/generic_task/test_config.yaml --context e2e_tests/tests/fixtures/generic_task
> created task <task-id>

  1. Kill Task:

$ det task kill <task-id>
> Successfully killed task: <task-id>

Pause & Unpause Task

  1. Create Task:

$ det task create e2e_tests/tests/fixtures/generic_task/test_config.yaml --context e2e_tests/tests/fixtures/generic_task
> created task <task-id>

  1. Kill Task:

$ det task pause <task-id>
> Successfully paused task: <task-id>

  1. Verify Pause:

$ det task logs -f

  1. Unpause Task:

$ det task unpause <task-id>
> Successfully unpaused task: <task-id>

  1. Verify Task Completion:
    $ det task logs -f

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-9945
DET-9986
DET-9987
DET-10016
DET-10017
DET-10002
DET-10102
DET-10052
DET-10093

Copy link

netlify bot commented Jan 22, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit aae7fee
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/65ca46bf13af5c0009acd0bc
😎 Deploy Preview https://deploy-preview-8724--determined-ui.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

master/pkg/model/generic_task_config.go Outdated Show resolved Hide resolved
schemas/expconf/v0/resources.json Outdated Show resolved Hide resolved
harness/determined/cli/task.py Outdated Show resolved Hide resolved
master/internal/api_generic_tasks.go Outdated Show resolved Hide resolved
@AmanuelAaron AmanuelAaron changed the title Generic task final feat: Generic Tasks Jan 25, 2024
Copy link

codecov bot commented Jan 25, 2024

Codecov Report

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

Comparison is base (6206bde) 47.84% compared to head (aae7fee) 47.69%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8724      +/-   ##
==========================================
- Coverage   47.84%   47.69%   -0.16%     
==========================================
  Files        1061     1065       +4     
  Lines      168144   169636    +1492     
  Branches     2238     2238              
==========================================
+ Hits        80447    80901     +454     
- Misses      87539    88577    +1038     
  Partials      158      158              
Flag Coverage Δ
backend 43.45% <20.23%> (-0.51%) ⬇️
harness 64.11% <29.69%> (-0.21%) ⬇️
web 42.55% <46.40%> (+0.02%) ⬆️

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

Files Coverage Δ
master/internal/core_experiment.go 61.23% <100.00%> (-0.18%) ⬇️
master/pkg/model/experiment_config.go 39.24% <ø> (ø)
master/pkg/tasks/task_command.go 0.00% <ø> (ø)
master/internal/command/authz_basic_impl.go 3.44% <0.00%> (-0.26%) ⬇️
master/internal/db/postgres_jobs.go 52.38% <60.00%> (-2.62%) ⬇️
master/pkg/model/job.go 0.00% <0.00%> (ø)
master/internal/api_command.go 47.08% <73.33%> (-1.33%) ⬇️
master/pkg/model/task.go 15.26% <0.00%> (-0.33%) ⬇️
master/internal/api_task.go 70.37% <62.50%> (+34.00%) ⬆️
master/internal/spec_util.go 75.71% <75.71%> (ø)
... and 7 more

... and 4 files with indirect coverage changes

@AmanuelAaron AmanuelAaron marked this pull request as ready for review January 25, 2024 21:40
@AmanuelAaron AmanuelAaron requested review from a team as code owners January 25, 2024 21:40
e2e_tests/tests/fixtures/configuration/run.py Outdated Show resolved Hide resolved
e2e_tests/tests/fixtures/configuration/test_config.yaml Outdated Show resolved Hide resolved
harness/determined/cli/task.py Show resolved Hide resolved
harness/determined/cli/task.py Outdated Show resolved Hide resolved
harness/determined/cli/task.py Show resolved Hide resolved
master/internal/spec_util.go Outdated Show resolved Hide resolved
master/internal/spec_util.go Outdated Show resolved Hide resolved
master/pkg/schemas/expconf/experiment_config.go Outdated Show resolved Hide resolved
master/internal/api_generic_tasks.go Outdated Show resolved Hide resolved
@ioga
Copy link
Contributor

ioga commented Feb 7, 2024

If you want to test context directories, make a specific test for it, and use one of our FileTrees to define the relevant files right in the test. Those sorts of tests are much easier to read because all the definitions are in one place (and you don't really have anything sophisticated to put into the fixture, like a whole model definition).

then you can't reuse them for a manual test.

@rb-determined-ai
Copy link
Member

then you can't reuse them for a manual test.

I don't think these particular fixtures offer enough to be useful to a manual test.

@ioga
Copy link
Contributor

ioga commented Feb 7, 2024

I don't think these particular fixtures offer enough to be useful to a manual test.

they're better than nothing, and one can extend them as needed: rather than creating something from scratch every time, or having a stashed example somewhere, I can cd into that fixture and edit it the way I need to.

alternatively, what place do we have where we can put an example that is too experimental for an end user?

@rb-determined-ai
Copy link
Member

they're better than nothing

I would argue these fixtures are not better than nothing, because they're so thin. I would have to go read the test to even locate where the fixture is, and then read the test to figure out how it would be called. I'd have to mentally convert the SDK and read_context calls into their CLI equivalents. I wouldn't do that, I'd just mkdir junk && cd junk and construct these files from scratch.

Not to mention that eliminating the fixtures will actually simplify the tests being written here. There's no good reason why these any of these tests need to be reading contexts manually in the first place.

I'm not arguing against all fixtures. But I do think if you can easily replace the fixture with inline strings then that's an indication the fixture isn't sophisticated enough to justify the fixture.

alternatively, what place do we have where we can put an example that is too experimental for an end user?

Don't we have an unsupported experiments repo now? I'm not sure I see the relevance of that question to the conversation though.

@ioga
Copy link
Contributor

ioga commented Feb 7, 2024

I would have to go read the test to even locate where the fixture is, and then read the test to figure out how it would be called.

you can send a link to someone pointing to the fixture directory, and they can cd into it and not edit any files. this PR has the command line in the comment, we could put README.md next to the fixture with the det task create ... command in it if we really wanted to. it seems much like a much faster way to run something than to build something from scratch.

file fixture can be reused. synthetic FileTrees in the tests is strictly confined to the test, and extracting them will take 20 minutes. if you have only 2 static fixtures, not 25 combinations generated on the fly, I do not see the appeal.

Don't we have an unsupported experiments repo now? I'm not sure I see the relevance of that question to the conversation though.

The feature must have at least some usable example we can put somewhere. This fixture was supposed to be this example. I am not sure the new examples repo is an appropriate place for "experimental" stuff but I am fine using it.

@rb-determined-ai
Copy link
Member

It sounds like we are not going to agree on the value of tiny fixtures. But this isn't a hill I want to die on.

The feature must have at least some usable example we can put somewhere. This fixture was supposed to be this example. I am not sure the new examples repo is an appropriate place for "experimental" stuff but I am fine using it.

Ok, a fixture is a reasonable solution to meet these constraints.

Copy link
Member

@rb-determined-ai rb-determined-ai left a comment

Choose a reason for hiding this comment

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

e2e tests look good

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.

stamp since ryan and ilia reviewed also (also i think i reviewed every commit in this as it landed into the feature branch)

@AmanuelAaron AmanuelAaron merged commit e341e27 into main Feb 12, 2024
74 of 87 checks passed
@AmanuelAaron AmanuelAaron deleted the generic-task-final branch February 12, 2024 22:12
maxrussell pushed a commit that referenced this pull request Mar 21, 2024
* Create Generic Task

* Get Task Config

* Fork Task

* Create Child Task

* small fix

* fix fork tree merge

* Kill task

* fix kill merge

* remove duplicates

* fix slots

* error for non-generic tasks

* fix bindings and kill call

* Pause & Resume Tasks

* pause/resume merge fixes

* bindings clean build

* Apply suggestions from code review

Co-authored-by: Ilia Glazkov <[email protected]>

* remove duplicate test

* unify migrations

* formatting

* hide from -h

* pause -> unpause

* Create Generic Task

* Get Task Config

* Fork Task

* Create Child Task

* small fix

* fix fork tree merge

* Kill task

* fix kill merge

* remove duplicates

* fix slots

* error for non-generic tasks

* fix bindings and kill call

* Pause & Resume Tasks

* pause/resume merge fixes

* bindings clean build

* remove duplicate test

* unify migrations

* Apply suggestions from code review

Co-authored-by: Ilia Glazkov <[email protected]>

* formatting

* hide from -h

* pause -> unpause

* fix single node check and naming

* fix injection

* lint + test fixes

* fixes

* no pause in task.py

* go lint

* add e2e test for generic tasks

* isort test

* fix test

* shorten test task run time

* single node default true in api_command

* move to Bun

* fix pb

* lint migrations

* fix missing order by

* fix intg test

* Apply suggestions from code review

Co-authored-by: Ilia Glazkov <[email protected]>

* fix merge

* fix style

* add task state check

* fix comment

* update migrations

* fix todo

* remove action for get task config

* update e2e test

* reduce test fixture time

* remove run python file

* add completion tests

* fix imports

* test lint

* update tests

* fix api.ts conflict

* send dummy job for generic tasks

* set weight

* lint migrations

* implement v1Job

---------

Co-authored-by: Ilia Glazkov <[email protected]>
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.

5 participants