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

bug fix: unused arg for batch step --help #1817

Merged
merged 4 commits into from May 8, 2024

Conversation

mae5357
Copy link
Contributor

@mae5357 mae5357 commented Apr 29, 2024

addresses: #1771

issue:

python hello.py batch step --help command was breaking due to wrong type for click.Choice arg.

change:

setting default arg for ubf-context from None to "none" in "batch step" make it compatible with click.Choice()

however, ubf-context is not needed for the "step" function when using aws-batch, so we can safely remove altogether.

@mae5357 mae5357 changed the title Mae5357/bug fix/batch step help bug fix: TypeError when running python hello.py batch step --help Apr 29, 2024
@mae5357 mae5357 changed the title bug fix: TypeError when running python hello.py batch step --help bug fix: unused arg for batch step --help Apr 29, 2024
madhur-ob
madhur-ob previously approved these changes Apr 29, 2024
@romain-intel
Copy link
Contributor

romain-intel commented Apr 30, 2024

If you want to keep the functionality, the proper line should read something like:

type=click.Choice(["none", UBF_CONTROL, UBF_TASK]),

I was under the impression that ubf may be used by parallel so may be worth keeping around.

However, no issues either way so feel free to merge when you are happy with it.

@nflx-mf-bot
Copy link
Collaborator

Testing[735] @ 4e64862

@nflx-mf-bot
Copy link
Collaborator

Testing[735] @ 4e64862 had 6 FAILUREs.

@mae5357
Copy link
Contributor Author

mae5357 commented May 2, 2024

@romain-intel i can revert to if you want 244ff31

I was under the impression that ubf may be used by parallel so may be worth keeping around.

fwiw, the step() function this is decorating doesn't take ubf-context as an argument. So there's not a possibility this arg is being used (here).

(edit: i don't have permission to merge)

@saikonen
Copy link
Collaborator

saikonen commented May 8, 2024

Finally had time to test this, and as Romain mentioned, @parallel somewhat depends on the option existing, even if it is not explicitly used. Removing the option completely breaks parallel deco on batch.

Here's a simple test flow for the issue:

from metaflow import step, FlowSpec, parallel

class UBFTest(FlowSpec):
    @step
    def start(self):
        print("Starting 👋")
        self.next(self.process, num_parallel=4)
    
    @parallel
    @step
    def process(self):
        print(f"processing input: {self.input}")
        self.next(self.join)

    @step
    def join(self, inputs):
        self.next(self.end)

    @step
    def end(self):
        print("Done! 🏁")


if __name__ == "__main__":
    UBFTest()

Comment on lines -153 to -154
# TODO: Maybe remove it altogether since it's not used here
@click.option("--ubf-context", default=None, type=click.Choice([None, "ubf_control"]))
Copy link
Collaborator

Choose a reason for hiding this comment

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

removing this completely leads to @parallel breaking on batch. Keep the original fix instead and change comment to

# NOTE: ubf-context is not explicitly used, but @parallel decorator tries to pass this so keep it for now.

@mae5357 mae5357 requested a review from saikonen May 8, 2024 16:31
# TODO: Maybe remove it altogether since it's not used here
@click.option("--ubf-context", default=None, type=click.Choice([None, "ubf_control"]))
# NOTE: ubf-context is not explicitly used, but @parallel decorator tries to pass this so keep it for now
@click.option("--ubf-context", default=None, type=click.Choice(["none", "ubf_control"]))
Copy link
Contributor

Choose a reason for hiding this comment

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

I would still recommend: type=click.Choice(["none", UBF_CONTROL, UBF_TASK]),

@saikonen saikonen merged commit 817eba4 into Netflix:master May 8, 2024
26 checks passed
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.

None yet

5 participants