[Ready for Review] Make TOGGLE_DECOSPECS only add default decorator once. #2097

darinyu · 2024-10-13T23:51:19Z

This is needed when we add value to DECOSPECS (or TOGGLE_DECOPSECS).

Changes:

update the METAFLOW_DECOSPECS (from config) to METAFLOW_DEFAULT_DECOSPECS. This will no longer collide with user override METAFLOW_DECOSPECS which translate to --with
remove config_merge_cb from start but move it to run. This is because "start" is called both in beginning of runtime and start of each task subprocess. Moving it to run will also attach decorator to each step.

Before the change:
when we use TOGGLE_DECOSPECS and run a flow, it first inject X as decorator at flow level (by this config_merge_cb. It then get passed to the subprocess when we run step. When step starts, it injects this X again, resulting in double decorators

After the change
when we use TOGGLE_DECOSPECS and run a flow, it no longer inject X (because this config_merge_cb is no longer in start in cli.py), When subprocess starts to run step, we inject this X to decorator (within step command line execution).

To test:

The following should include default card (once) in each step.

METAFLOW_DEFAULT_DECOSPECS=card python ./helloworld.py run

romain-intel · 2025-02-13T18:37:32Z

metaflow/metaflow_config.py

@@ -285,7 +285,7 @@
 ###
 # Format is a space separated string of decospecs (what is passed
 # using --with)
-DECOSPECS = from_conf("DECOSPECS", "")
+DECOSPECS = from_conf("DEFAULT_DECOSPECS", "")


To be consistent, I would rename this: DEFAULT_DECOSPECS = from_conf("DEFAULT_DECOSPECS", "")

romain-intel · 2025-02-13T18:38:07Z

metaflow/plugins/cards/card_modules/basic.py

@@ -739,7 +739,9 @@ class BlankCard(MetaflowCard):

    type = "blank"

-    def __init__(self, options=dict(title=""), components=[], graph=None, **kwargs):
+    def __init__(
+        self, options=dict(title=""), components=[], graph=None, flow=None, **kwargs


unrelated change? I think it's valid though but jsut checking.

its old, removed

romain-intel · 2025-02-13T18:38:34Z

metaflow/cli_components/step_cmd.py

@@ -7,6 +7,7 @@
 from ..task import MetaflowTask
 from ..unbounded_foreach import UBF_CONTROL, UBF_TASK
 from ..util import decompress_list
+from ..metaflow_config import DECOSPECS


this would therefore be DEFAULT_DECOSPECS

romain-intel · 2025-02-13T18:48:11Z

metaflow/cli_components/step_cmd.py

@@ -127,6 +128,20 @@ def step(
        raise CommandException("Function *%s* is not a step." % step_name)
    echo("Executing a step, *%s*" % step_name, fg="magenta", bold=False)

+    decospecs = tuple(DECOSPECS.split())
+    if decospecs:
+        decorators._attach_decorators_to_step(func, decospecs)


I think this needs to be done earlier when we do _attach_decorators because here you may be missing out on some of the lifecycle hooks. The issue though is we still need to prevent them from loading the second time around. What I would suggest (and not 100% sure this would work) is that when we launch the subprocess (or any of the commands), we explicitly set METAFLOW_DEFAULT_DECOSPECS to an empty value since all decospecs will now be passed using METAFLOW_DECOSPECS or on the CL.

I just realized that we can rely on run_cmd because it also add decorator to downstream. can you help review updated PR?

romain-intel · 2025-02-13T18:48:57Z

metaflow/decorators.py

+            # then check if we see any duplicate attributes. If we do,
+            # we ignore the new decorator and continue.
+            for deco in func.decorators:
+                if deco.name == decotype.name and deco.attributes == kwargs:


I think this is where we can merge with defaults so that we can properly compare.

weird enough, we dont reach this function now (maybe some changes to the flow or maybe I hallucinated, I need to take a deeper look on what's going on with the check

updat econfig merge add more temp code

metaflow/cli_components/run_cmds.py

npow · 2025-02-13T20:18:22Z

how did you test this PR?

darinyu · 2025-02-13T20:25:23Z

how did you test this PR?

added the test command in the description

npow · 2025-02-14T22:56:23Z

Can we add an actual test for this?

Edit: just realized this is in the OSS repo. Maybe add a UX test on our end?

darinyu · 2025-02-19T18:28:30Z

Can we add an actual test for this?

Edit: just realized this is in the OSS repo. Maybe add a UX test on our end?

yes, I made this change so that we can add default metrics card for runs with GPU, will update UX test for that

romain-intel

I think this is fine. I think we can further simplify though.

romain-intel · 2025-02-20T01:25:34Z

metaflow/cli_components/run_cmds.py

+    # from_conf)
+
+    # Special case where DEFAULT_DECOSPECS and value are the same. This happens
+    # when there is no --with option at the TL and DEFAULT_DECOSPECS is read from


nit: it's not the TL but more at the run/resume level.

romain-intel · 2025-02-20T01:25:56Z

metaflow/cli_components/run_cmds.py

+    # when there is no --with option at the TL and DEFAULT_DECOSPECS is read from
+    # the env var. In this case, click also passes it as value
+    splits = DEFAULT_DECOSPECS.split()
+    if len(splits) == len(value) and all([a == b for (a, b) in zip(splits, value)]):


Looking at this: https://click.palletsprojects.com/en/stable/advanced/#callback-evaluation-order, the callback is called even if there is no value passed in but I don't think it will have the value DEFAULT_DECOSPECS since now that is not the same name as before. I think we can get rid this line.

updated, and also comments.

darinyu changed the title ~~do not let default decospec overwrite existing "--with"~~ [WIP] do not let default decospec overwrite existing "--with" Oct 13, 2024

darinyu force-pushed the default_decospecs branch 2 times, most recently from bf90146 to c0c0299 Compare December 12, 2024 18:14

darinyu changed the title ~~[WIP] do not let default decospec overwrite existing "--with"~~ [Ready for review] Make TOGGLE_DECOSPECS only add default decorator once. Dec 12, 2024

darinyu force-pushed the default_decospecs branch from 10b9856 to 2ba22db Compare January 21, 2025 20:23

darinyu changed the title ~~[Ready for review] Make TOGGLE_DECOSPECS only add default decorator once.~~ [WIP] Make TOGGLE_DECOSPECS only add default decorator once. Feb 5, 2025

romain-intel reviewed Feb 13, 2025

View reviewed changes

darinyu force-pushed the default_decospecs branch from 2ba22db to b7c147f Compare February 13, 2025 19:25

darinyu added 7 commits February 13, 2025 19:28

do not let default decospec overwrite existing --with

2236e44

updat econfig merge add more temp code

change to default_decospecs

e4e1909

remove unnecessary space

6416592

avoid dup deco

57960fc

fix card initialization

94f5079

update decospecs insert time

deec08a

update to DEFAULT_DECOSPECS

92b248f

darinyu force-pushed the default_decospecs branch from 1feac60 to 92b248f Compare February 13, 2025 19:28

darinyu changed the title ~~[WIP] Make TOGGLE_DECOSPECS only add default decorator once.~~ [Ready for Review] Make TOGGLE_DECOSPECS only add default decorator once. Feb 13, 2025

npow reviewed Feb 13, 2025

View reviewed changes

metaflow/cli_components/run_cmds.py Outdated Show resolved Hide resolved

update according to comments

e7ca046

darinyu requested review from romain-intel and npow February 19, 2025 18:27

romain-intel previously approved these changes Feb 20, 2025

View reviewed changes

update according to comments

ebacbb5

darinyu dismissed romain-intel’s stale review via ebacbb5 February 20, 2025 18:21

darinyu requested a review from romain-intel February 20, 2025 18:21

romain-intel approved these changes Feb 21, 2025

View reviewed changes

darinyu merged commit f5b1502 into master Feb 21, 2025
29 checks passed

darinyu deleted the default_decospecs branch February 21, 2025 17:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Ready for Review] Make TOGGLE_DECOSPECS only add default decorator once. #2097

[Ready for Review] Make TOGGLE_DECOSPECS only add default decorator once. #2097

darinyu commented Oct 13, 2024 •

edited

Loading

romain-intel Feb 13, 2025

darinyu Feb 13, 2025

romain-intel Feb 13, 2025

darinyu Feb 13, 2025

romain-intel Feb 13, 2025

darinyu Feb 13, 2025

romain-intel Feb 13, 2025

darinyu Feb 13, 2025

romain-intel Feb 13, 2025

darinyu Feb 13, 2025

npow commented Feb 13, 2025

darinyu commented Feb 13, 2025

npow commented Feb 14, 2025 •

edited

Loading

darinyu commented Feb 19, 2025

romain-intel left a comment

romain-intel Feb 20, 2025

darinyu Feb 20, 2025

romain-intel Feb 20, 2025

darinyu Feb 20, 2025

[Ready for Review] Make TOGGLE_DECOSPECS only add default decorator once. #2097

[Ready for Review] Make TOGGLE_DECOSPECS only add default decorator once. #2097

Conversation

darinyu commented Oct 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npow commented Feb 13, 2025

darinyu commented Feb 13, 2025

npow commented Feb 14, 2025 • edited Loading

darinyu commented Feb 19, 2025

romain-intel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darinyu commented Oct 13, 2024 •

edited

Loading

npow commented Feb 14, 2025 •

edited

Loading