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

Exclude start stage tasks from existing tasks #713

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

Conversation

eu9ene
Copy link
Collaborator

@eu9ene eu9ene commented Jun 28, 2024

This fixes the edge case where we have alignments-original -> alignments-backtranslated both marked as stage: alignments-teacher and want to restart both of them. We should probably split them later to two stages but not now because it breaks the ability to reuse previous graphs.

@eu9ene eu9ene requested a review from bhearsum June 28, 2024 21:57
@eu9ene eu9ene requested a review from a team as a code owner June 28, 2024 21:57
Copy link
Collaborator

@bhearsum bhearsum left a comment

Choose a reason for hiding this comment

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

Is this a generally good thing, or could there be groups in previous_group_ids where we'd want to overwrite one task from it but not another? The safer way to do this may be to allow a list of labels, substrings or prefixes to be specified in the training config, and filter anything matching them out of existing_tasks.

(We do something similar for optimizations. We can't directly use that here, but we could use the same technique to keep things flexible.

@eu9ene
Copy link
Collaborator Author

eu9ene commented Jul 9, 2024

Is this a generally good thing, or could there be groups in previous_group_ids where we'd want to overwrite one task from it but not another? The safer way to do this may be to allow a list of labels, substrings or prefixes to be specified in the training config, and filter anything matching them out of existing_tasks.

(We do something similar for optimizations. We can't directly use that here, but we could use the same technique to keep things flexible.

@bhearsum It probably depends on the situation but usually, I want to restart all the tasks with start_stage. This is basically a hotfix so that I can at least have a way to schedule what I need. So I suggest we merge it for now and do a more comprehensive analysis in #719

@bhearsum
Copy link
Collaborator

bhearsum commented Jul 9, 2024

Is this a generally good thing, or could there be groups in previous_group_ids where we'd want to overwrite one task from it but not another? The safer way to do this may be to allow a list of labels, substrings or prefixes to be specified in the training config, and filter anything matching them out of existing_tasks.
(We do something similar for optimizations. We can't directly use that here, but we could use the same technique to keep things flexible.

@bhearsum It probably depends on the situation but usually, I want to restart all the tasks with start_stage. This is basically a hotfix so that I can at least have a way to schedule what I need. So I suggest we merge it for now and do a more comprehensive analysis in #719

I think it's probably fine on release, seeing as it doesn't necessarily live forever there. I'm more hesitant to take a quick hack on main. Ultimately I consider this up to you and @gregtatum though, so I'll defer to him on this PR.

@bhearsum bhearsum requested a review from gregtatum July 9, 2024 21:11
Copy link
Member

@gregtatum gregtatum left a comment

Choose a reason for hiding this comment

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

Approving for the release branch. Can you rewrite the merge commit to have a message like "RELEASE ONLY: Exclude start stage tasks from existing tasks" so that we don't accidentally cherry pick it back?

@eu9ene
Copy link
Collaborator Author

eu9ene commented Jul 16, 2024

I think I'm doing fine with just cherry-picking this into a separate branch so we don't have to merge it at all if you have doubts it's the right solution here

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