Better logs for @conda/@pypi #2080

savingoyal · 2024-10-05T21:12:44Z

also drive by roughly ~40% speed up on benchmark flows

savingoyal · 2024-10-07T16:15:56Z

metaflow/plugins/pypi/micromamba.py

    def info(self):
        return self._call(["config", "list", "-a"])

+    @functools.lru_cache(maxsize=None)


to check if there is any unintended consequence of this

There is an implicit None returned from this call which will be cached, if the environment has not been formed yet. This might cause problems in some cases

savingoyal · 2024-10-07T16:16:22Z

metaflow/plugins/pypi/bootstrap.py

+
+    with concurrent.futures.ThreadPoolExecutor(max_workers=10) as executor:
+        # install micromamba, download conda and pypi packages in parallel
+        future_install_micromamba = executor.submit(install_micromamba, architecture)


check if download from datastore possible

savingoyal · 2024-10-07T16:21:41Z

metaflow/cli.py

@@ -960,7 +1027,7 @@ def start(
    ctx.obj.environment = [
        e for e in ENVIRONMENTS + [MetaflowEnvironment] if e.TYPE == environment
    ][0](ctx.obj.flow)
-    ctx.obj.environment.validate_environment(ctx.obj.logger, datastore)
+    ctx.obj.environment.validate_environment(echo, datastore)


take care of symmetric changes for fast bakery

savingoyal · 2024-10-07T16:24:47Z

metaflow/plugins/pypi/conda_environment.py

+                    executor.submit(self.solvers[solver].create, *result)
+                    if storage:
+                        # parallel cache
+                        executor.submit(cache, storage, [result], solver)


pypi execution can start a lot earlier

need to be careful with the timing though, as a lot of pypi steps rely on
self.micromamba.path_to_environment(id_) which can get stuck to None if called too early.

savingoyal · 2024-10-07T16:25:36Z

metaflow/cli.py

+        "underline": kwargs.get("underline", False),
+    }
+
+    if animate:


check how animate and overwrite shows up in runtime logs and notebooks

Yes please!

notebooks seem broken still, animate produces new lines for each frame, even though the \r echoing by itself works outside of Metaflow runtime.

romain-intel

Didn't look at non CLI part.

One general comment as well is we have too many echo functions (I was bitten already). It would be nice to clean that up and make all of them take the same arguments.

I like the idea of adding a spinner like that though.

romain-intel · 2024-10-10T02:42:38Z

metaflow/cli.py

+        _animation_stop.clear()
+        click.echo("\r", nl=False, err=kwargs["err"])
+
+    if indent:


simplify: if indent and not animate

romain-intel · 2024-10-10T02:43:11Z

metaflow/cli.py

+            line = INDENT + line
+
+    animation_kwargs = {
+        "fg": kwargs.get("fg"),


fallbacks needed to be safe.

romain-intel · 2024-10-10T02:43:22Z

metaflow/cli.py

+        "underline": kwargs.get("underline", False),
+    }
+
+    if animate:


Yes please!

romain-intel · 2024-10-10T02:43:48Z

metaflow/cli.py

+
+    if animate:
+        if animate is True:
+            get_frame = lambda line: f"{next(_default_spinner)} {line}"


f-string not supported in 3.5

romain-intel · 2024-10-10T02:45:18Z

metaflow/cli.py

+        if animate is True:
+            get_frame = lambda line: f"{next(_default_spinner)} {line}"
+        else:
+            get_frame = animate


confused here -- what is animate? It doesn't seem to be a simple bool here so a bit confused.

romain-intel · 2024-10-10T02:48:05Z

metaflow/cli.py

+
+
+def _animate(get_frame, line, err, kwargs, indent):
+    while not _animation_stop.is_set():


this doesn't seem to respect the same arguments as the non animate case.

saikonen · 2024-10-10T10:10:51Z

metaflow/cli.py

+        "underline": kwargs.get("underline", False),
+    }
+
+    if animate:


notebooks seem broken still, animate produces new lines for each frame, even though the \r echoing by itself works outside of Metaflow runtime.

saikonen · 2024-10-10T10:13:09Z

metaflow/plugins/pypi/conda_environment.py

-                storage = self.datastore(_datastore_packageroot(self.datastore, echo))
-                cache(storage, results, solver)
-        self.logger("Virtual environment(s) bootstrapped!")
+                # parallel solves


Can you also rename the PR as the scope of this is beyond simple log changes at this point.

saikonen · 2024-10-10T10:26:34Z

metaflow/plugins/pypi/micromamba.py

    def info(self):
        return self._call(["config", "list", "-a"])

+    @functools.lru_cache(maxsize=None)


There is an implicit None returned from this call which will be cached, if the environment has not been formed yet. This might cause problems in some cases

saikonen · 2024-10-10T10:27:56Z

metaflow/plugins/pypi/micromamba.py

        if os.path.exists(f"{prefix}/fake.done"):
            return

+        # somewhat expensive check
+        # TODO: make this less expensive
+        if self.path_to_environment(id_, platform):


during forming a new environment, would this not cache the path to environment as None due to it not being ready yet?

saikonen · 2024-10-10T10:31:05Z

metaflow/plugins/pypi/conda_environment.py

+                    executor.submit(self.solvers[solver].create, *result)
+                    if storage:
+                        # parallel cache
+                        executor.submit(cache, storage, [result], solver)


need to be careful with the timing though, as a lot of pypi steps rely on
self.micromamba.path_to_environment(id_) which can get stuck to None if called too early.

speed up remote bootstrap for @pypi/@conda

40f4b0f

savingoyal force-pushed the newr branch from 4acac81 to 40f4b0f Compare October 5, 2024 21:14

savingoyal added 3 commits October 5, 2024 14:16

remove print statements

ef81307

more speed ups

b870280

even faster

3aedfb7

savingoyal changed the title ~~speed up container bootstrap for @pypi/@conda~~ speed up bootstrap for @pypi/@conda Oct 6, 2024

savingoyal added 3 commits October 5, 2024 23:18

better logs

6d11fff

better logs

8c90b87

cleanup

148f567

savingoyal changed the title ~~speed up bootstrap for @pypi/@conda~~ Better logs for @conda/@pypi Oct 6, 2024

savingoyal commented Oct 7, 2024

View reviewed changes

romain-intel reviewed Oct 10, 2024

View reviewed changes

saikonen reviewed Oct 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better logs for @conda/@pypi #2080

Better logs for @conda/@pypi #2080

savingoyal commented Oct 5, 2024 •

edited

Loading

savingoyal Oct 7, 2024

saikonen Oct 10, 2024

savingoyal Oct 7, 2024

savingoyal Oct 7, 2024

savingoyal Oct 7, 2024

saikonen Oct 10, 2024

savingoyal Oct 7, 2024

romain-intel Oct 10, 2024

saikonen Oct 10, 2024

romain-intel left a comment

romain-intel Oct 10, 2024

romain-intel Oct 10, 2024

romain-intel Oct 10, 2024

romain-intel Oct 10, 2024

romain-intel Oct 10, 2024

romain-intel Oct 10, 2024

saikonen Oct 10, 2024

saikonen Oct 10, 2024

saikonen Oct 10, 2024

saikonen Oct 10, 2024

saikonen Oct 10, 2024



		def _animate(get_frame, line, err, kwargs, indent):
		while not _animation_stop.is_set():

Better logs for @conda/@pypi #2080

Are you sure you want to change the base?

Better logs for @conda/@pypi #2080

Conversation

savingoyal commented Oct 5, 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

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

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

savingoyal commented Oct 5, 2024 •

edited

Loading