-
Notifications
You must be signed in to change notification settings - Fork 35
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
Introducing a pythonic CLI #5
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you try rebasing on the latest main
? That should update some of the deleted docstrings.
@@ -1,72 +1,50 @@ | |||
# SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
# SPDX-License-Identifier: Apache-2.0 | |||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we remove this header?
@@ -317,7 +320,13 @@ def _save_tasks(self): | |||
with open(os.path.join(self._exp_dir, self.__class__._TASK_FILE), "w+") as f: | |||
json.dump(serialized_tasks, f) | |||
|
|||
if "__main__" in sys.modules: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work for remote executors as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is called on the node that's configuring the experiment. The output is put inside the experiment dir, which I am under the impression is accessible for the remote executor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the necessary files are accessible remotely, which currently just includes the configs and the code. You'll have to edit the executors to include the copying of this file.
4af05bd
to
c2c565d
Compare
is_target_default: bool = False, | ||
name: Optional[str] = None, | ||
namespace: Optional[str] = None, | ||
) -> F: ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
is_target_default: bool = False, | ||
name: Optional[str] = None, | ||
namespace: Optional[str] = None, | ||
) -> F: ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
@@ -95,6 +448,20 @@ | |||
return app | |||
|
|||
|
|||
@runtime_checkable | |||
class EntrypointProtocol(Protocol): | |||
def cli_entrypoint(self) -> "Entrypoint": ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
@@ -61,7 +62,9 @@ | |||
def create() -> typer.Typer: | |||
app = typer.Typer() | |||
|
|||
CLI(launch).cli(app) | |||
from nemo_run.cli.api import Entrypoint |
Check notice
Code scanning / CodeQL
Cyclic import Note
nemo_run.cli.api
from rich.console import Console | ||
from typer.testing import CliRunner | ||
|
||
import nemo_run as run |
Check notice
Code scanning / CodeQL
Module is imported with 'import' and 'import from' Note test
62de001
to
fb81aeb
Compare
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
476bcee
to
bca6553
Compare
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
@@ -317,7 +320,13 @@ def _save_tasks(self): | |||
with open(os.path.join(self._exp_dir, self.__class__._TASK_FILE), "w+") as f: | |||
json.dump(serialized_tasks, f) | |||
|
|||
if "__main__" in sys.modules: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the necessary files are accessible remotely, which currently just includes the configs and the code. You'll have to edit the executors to include the copying of this file.
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
Signed-off-by: Marc Romeyn <[email protected]>
No description provided.