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

Adding yaml + lazy execution #53

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Adding yaml + lazy execution #53

wants to merge 7 commits into from

Conversation

marcromeyn
Copy link
Collaborator

No description provided.

@ryxli
Copy link

ryxli commented Oct 2, 2024

Wondering when this could get merged?

src/nemo_run/cli/api.py Dismissed Show dismissed Hide dismissed
src/nemo_run/core/serialization/yaml.py Fixed Show fixed Hide fixed
src/nemo_run/lazy.py Dismissed Show dismissed Hide dismissed
src/nemo_run/lazy.py Fixed Show fixed Hide fixed
Signed-off-by: Marc Romeyn <[email protected]>
test/test_lazy.py Dismissed Show dismissed Hide dismissed
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
test/test_lazy.py Fixed Show fixed Hide fixed
Signed-off-by: Marc Romeyn <[email protected]>
@marcromeyn marcromeyn marked this pull request as ready for review October 28, 2024 14:54
Signed-off-by: Marc Romeyn <[email protected]>
@@ -253,6 +253,11 @@ def __init__(
bind_args: bool = True,
**kwargs,
):
# Handle dict types by converting to _kwargs_to_dict function
if fn_or_cls is dict or (hasattr(fn_or_cls, "__origin__") and fn_or_cls.__origin__ is dict):
fn_or_cls = _kwargs_to_dict # type: ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can just set this as dict

@@ -550,3 +566,7 @@ def _try_set_all(config: _BuildableT, _walk: bool = False, **kwargs) -> _Buildab
pass

return config


def _kwargs_to_dict(**kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for this if fn_or_cls is set to dict

@@ -262,6 +267,17 @@ def __init__(

super().__init__(fn_or_cls, *args, **new_kwargs)

@classmethod
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this required? Config object handles dicts.

In [1]: import fiddle as fdl

In [2]: fdl.Config(dict, a=5)
Out[2]: <Config[dict(a=5)]>

In [3]: a = fdl.Config(dict, a=5)

In [4]: fdl.build(a)
Out[4]: {'a': 5}

@@ -61,6 +61,9 @@ def dryrun_fn(


def direct_run_fn(task: Partial | Script, dryrun: bool = False):
if hasattr(task, "__is_lazy__"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be is_lazy to be consistent with the others?

@@ -0,0 +1,93 @@
# SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be moved to the test folder.

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