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

Unexpected path attr set for SmartSim entities when passing an Optional path param #556

Open
3 tasks
MattToast opened this issue Apr 18, 2024 · 0 comments
Open
3 tasks
Labels
area: api Issues related to API changes bug: minor A minor bug good first issue Issue that is ideal for first-time contributors short task Issues that can be completed and reviewed quickly

Comments

@MattToast
Copy link
Member

Description

Currently the path parameter to the __init__ methods of Ensemble/Model/Orchestrator are type hinted as so (or equivalent):

def __init__(
    self,
    ...
    path: str | None = os.getcwd(),
    ...
) -> None:

However, if a user were to exercise the Optional type of path, an invalid path of the string "None" is set on the object, effectively leaving the object in an invalid state that cannot be launched.

How to reproduce

Consider the following repl

>>> from smartsim.database.orchestrator import Orchestrator
>>> from smartsim.entity.ensemble import Ensemble
>>> from smartsim.entity.model import Model
>>> from smartsim.settings import RunSettings
>>>
>>> Orchestrator(path=None).path
'None'
>>> rs = RunSettings("echo", ["hello", "world"])
>>> Ensemble("test-ens", {}, path=None, run_settings=rs, replicas=1).path
'None'
>>> Model("test-model", {}, rs, path=None).path
'None'

Expected behavior

The path should not be set to the string "None", but should instead be set to the current working directory. Whether that is the current working directory of at "compile time" or at "run time" remains to be decided.

Alternatively, we could have the path parameter not have a default value.

System

Any/All

Acceptance Criteria

  • Decide on if/what the default path for an Model/Ensemble/Orchestrator should be
  • Passing a path=None param to Model/Ensemble/Orchestrator results in a "valid" path, or disallowed entirely
  • If a default path is agreed upon, add tests to ensure that when path=None is specified the correct default path is set
@MattToast MattToast added area: api Issues related to API changes bug: minor A minor bug good first issue Issue that is ideal for first-time contributors short task Issues that can be completed and reviewed quickly labels Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: api Issues related to API changes bug: minor A minor bug good first issue Issue that is ideal for first-time contributors short task Issues that can be completed and reviewed quickly
Projects
None yet
Development

No branches or pull requests

1 participant