Netflix / metaflow

Open Source Platform for developing, scaling and deploying serious ML, AI, and data science systems
https://metaflow.org
Apache License 2.0
8.09k stars 762 forks source link

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

Closed mae5357 closed 4 months ago

mae5357 commented 5 months ago

addresses: https://github.com/Netflix/metaflow/issues/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.

romain-intel commented 5 months ago

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 commented 5 months ago

Testing[735] @ 4e648627ea6eb6535c0e28e80ce4daf8f1839ee7

nflx-mf-bot commented 5 months ago

Testing[735] @ 4e648627ea6eb6535c0e28e80ce4daf8f1839ee7 had 6 FAILUREs.

mae5357 commented 5 months ago

@romain-intel i can revert to if you want https://github.com/Netflix/metaflow/pull/1817/commits/244ff31c9c81ac811ef4b4499c4f819e74a028f3

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 commented 4 months ago

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()