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(dead code): remove unused invocation_finished on scheduler TaskCreationResult #1078

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

seriousben
Copy link
Member

@seriousben seriousben commented Dec 4, 2024

Context

CreationResult has an invocation_finished attribute that is misleading since it is not used even though we have lots of logic around it in the scheduler.

What

Delete the attribute and simplify code setting it.

Testing

Contribution Checklist

  • If the python-sdk was changed, please run make fmt in python-sdk/.
  • If the server was changed, please run make fmt in server/.
  • Make sure all PR Checks are passing.

@seriousben seriousben force-pushed the seriousben/clean-task-creation-result branch from dc80416 to 1dc1130 Compare December 4, 2024 02:21
namespace: &str,
compute_graph: &str,
invocation_id: &str,
invocation_finished: bool,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this removed? There were cases an invocation was still running but we were not creating new tasks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just a GitHub display issue. We still have that helper.

@@ -30,7 +30,6 @@ pub async fn handle_invoke_compute_graph(
compute_graph: event.compute_graph.clone(),
new_reduction_tasks: vec![],
processed_reduction_tasks: vec![],
invocation_finished: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be invocation_finished: true no?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is what is going to happen, but that attribute was not read anywhere.

In Create tasks, it checks for outstanding tasks to finalize.

@seriousben seriousben merged commit d2b817c into main Dec 5, 2024
5 checks passed
@seriousben seriousben deleted the seriousben/clean-task-creation-result branch December 5, 2024 01:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants