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.

Sign up for GitHub

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

Jump to bottom

Do not schedule execution of save operator if other computer operator… #1253

Merged
merged 1 commit into from
May 2, 2023
Merged

Do not schedule execution of save operator if other computer operator… #1253

merged 1 commit into from
May 2, 2023

Conversation

jpurusho65
Copy link
Contributor

…s fail in a DAG

Describe your changes and why you are making these changes

Schedule the Load (Save) operator for execution only if all other compute operators have succeeded

Related issue number (if any)

ENG-2661

Loom demo (if any)

Checklist before requesting a review

  • [x ] I have created a descriptive PR title. The PR title should complete the sentence "This PR...".
  • [x ] I have performed a self-review of my code.
  • I have included a small demo of the changes. For the UI, this would be a screenshot or a Loom video.
  • If this is a new feature, I have added unit tests and integration tests.
  • [ x] I have run the integration tests locally and they are passing.
  • I have run the linter script locally (See python3 scripts/run_linters.py -h for usage).
  • All features on the UI continue to work correctly.
  • Added one of the following CI labels:
    • run_integration_test: Runs integration tests
    • skip_integration_test: Skips integration tests (Should be used when changes are ONLY documentation/UI)

@jpurusho65 jpurusho65 requested a review from kenxu95 April 27, 2023 07:22
kenxu95
kenxu95 requested changes Apr 27, 2023
View reviewed changes
Copy link
Contributor

@kenxu95 kenxu95 left a comment

Choose a reason for hiding this comment

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

It looks like it'll work to me! Mainly had some naming/readability concerns. I also will need an integration test for this.

src/golang/lib/engine/aq_engine.go Outdated Show resolved Hide resolved
src/golang/lib/engine/aq_engine.go Show resolved Hide resolved
src/golang/lib/engine/aq_engine.go Outdated Show resolved Hide resolved
src/golang/lib/engine/aq_engine.go Show resolved Hide resolved
@jpurusho65 jpurusho65 force-pushed the eng-2661-always-execute-saves-last-during branch from 2e209db to 4d88042 Compare May 1, 2023 15:11
@jpurusho65 jpurusho65 requested a review from kenxu95 May 1, 2023 15:13
kenxu95
kenxu95 requested changes May 1, 2023
View reviewed changes
Copy link
Contributor

@kenxu95 kenxu95 left a comment

Choose a reason for hiding this comment

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

This seems to still be missing the new integration test for this?

@jpurusho65 jpurusho65 force-pushed the eng-2661-always-execute-saves-last-during branch 2 times, most recently from 92245da to 676da4b Compare May 1, 2023 20:06
@jpurusho65 jpurusho65 requested a review from kenxu95 May 1, 2023 20:07
@jpurusho65 jpurusho65 force-pushed the eng-2661-always-execute-saves-last-during branch from 676da4b to caa09f7 Compare May 1, 2023 20:27
@jpurusho65
Copy link
Contributor Author

This seems to still be missing the new integration test for this?

done

@jpurusho65 jpurusho65 closed this May 1, 2023
@jpurusho65 jpurusho65 reopened this May 1, 2023
@jpurusho65 jpurusho65 force-pushed the eng-2661-always-execute-saves-last-during branch 2 times, most recently from 740eeb4 to 009829c Compare May 2, 2023 05:11
@jpurusho65
Do not schedule execution of save operator if other computer operator…
b998201 
…s fail in a DAG

Add unit test case
Some linter related fixes for python llm module
@jpurusho65 jpurusho65 force-pushed the eng-2661-always-execute-saves-last-during branch from 009829c to b998201 Compare May 2, 2023 05:16
kenxu95
kenxu95 approved these changes May 2, 2023
View reviewed changes
Copy link
Contributor

@kenxu95 kenxu95 left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@jpurusho65 jpurusho65 added the run_integration_test Triggers integration tests label May 2, 2023
@jpurusho65 jpurusho65 merged commit b0d29d2 into main May 2, 2023
@jpurusho65 jpurusho65 deleted the eng-2661-always-execute-saves-last-during branch May 2, 2023 17:48
agiron123 pushed a commit that referenced this pull request May 9, 2023
@jpurusho65
Do not schedule execution of save operator if other computer operator… (
5de707a 
#1253)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kenxu95 kenxu95 kenxu95 approved these changes

Assignees
No one assigned
Labels
run_integration_test Triggers integration tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jpurusho65 @kenxu95