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

changed skip_if_completed_pre_run->task_completion_check_at_run #420

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mski-iksm
Copy link
Contributor

I've changed the parameter name and description, because it may be little confusing

@mski-iksm mski-iksm force-pushed the feature/rename_task_completion_check_at_run branch from fbf1bd6 to 7571ef9 Compare December 22, 2024 15:48
Copy link
Collaborator

@hirosassa hirosassa left a comment

Choose a reason for hiding this comment

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

👍

@hiro-o918
Copy link
Contributor

@mski-iksm
task_completion_check_at_run is confused with check_complete_on_run of luigi parameter.
So I renamed task_completion_check_at_run as skip_if_completed_pre_run because task_completion_check_at_run will be removed from gokart.TaskOnKart after migrating to gokart worker.

I suppose that task_completion_check_at_run should be more distinguishable from check_complete_on_run

Copy link
Contributor

@hiro-o918 hiro-o918 left a comment

Choose a reason for hiding this comment

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

LGTM

@mski-iksm
Copy link
Contributor Author

@hiro-o918

@mski-iksm task_completion_check_at_run is confused with check_complete_on_run of luigi parameter. So I renamed task_completion_check_at_run as skip_if_completed_pre_run because task_completion_check_at_run will be removed from gokart.TaskOnKart after migrating to gokart worker.

I suppose that task_completion_check_at_run should be more distinguishable from check_complete_on_run

Thank you for the comment.

It is true that task_completion_check_at_run is similar to check_complete_on_run, but I think it is the latter that is the misleading name.
check_complete_on_run is actually “tasks will also verify that their outputs exist when they finish running, and will fail immediately if the outputs are missing”. And I think task_completion_check_at_run is a more straightforward name since it does a complete_check at run time.

Therefore, I am thinking of leaving it as task_completion_check_at_run.

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.

3 participants