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

Split Config class #526

Open
al-rigazzi opened this issue Mar 20, 2024 · 1 comment
Open

Split Config class #526

al-rigazzi opened this issue Mar 20, 2024 · 1 comment
Labels
area: python Issues related to the Python Client good first issue Issue that is ideal for first-time contributors short task Issues that can be completed and reviewed quickly type: feature Issues that include feature request or feature idea type: refactor Issues focused on refactoring existing code

Comments

@al-rigazzi
Copy link
Collaborator

Description

The Config class should be split in smaller classes, each one representing a different family of configurable options.

Justification

The Config class holds information about most of SmartSim's configurable options, including those which can be set through environment variables.

It appears that Config currently holds information about:

  • database and redis installation
  • test options
  • WLM options
  • telemetry
    and in the future it will also hold options for Dragon (which will require adding # pylint: disable-next=too-many-public-methods to the class).

Implementation Strategy

I believe subclasses could be created, each one responsible for a different family of options/parameters, but other solutions may also be worth an investigation.

@al-rigazzi al-rigazzi added area: python Issues related to the Python Client good first issue Issue that is ideal for first-time contributors short task Issues that can be completed and reviewed quickly type: feature Issues that include feature request or feature idea type: refactor Issues focused on refactoring existing code labels Mar 20, 2024
@al-rigazzi al-rigazzi reopened this May 31, 2024
@MattToast
Copy link
Member

A Possible Simple Implementation Strategy

A naive implementation would be to simply break up the Config class into several similar, but smaller, classes over a more limited domain.

The three areas that I would recommend breaking the config class down into would be "Infrastructure" (e.g. where do I find dependencies), "Runtime" (e.g. how should SmartSim behave when running), and "Testing" (e.g. how should I run the test suite). Much like the current implementation of Config, all three of these classes could remain as largely static classes.

In order to maintain the interface where all elements of SmartSim interact with a single global CONFIG instance, I would recommend making instances of these new config classes attrs on the existing Config class. This change would roughly amount to something like this:

@dataclass(frozen=True)
class Config:
    infrastructure: _InfraConfg = field(default=_InfraConfig(...), init=False)
    runtime: _RuntimeConfig = field(default=_RuntimeConfig(...), init=False)
    testing: _TestingConfig = field(default=_TestingConfg(...), init=False)
#   ^^^^^^^
# Arguably these should just be defined in `conftest.py` as a fixture, because
# there is there is no need for a user or developer to get these setting
# outside of the test suite

CONFIG = Config()
CONFIG.infrastructure.deps_path  # "/path/to/some/dir"
CONFIG.runtime.job_manager_interval  # 10
CONFIG.testing.launcher  # "slurm"

A Possible More Complex Implementation Strategy

A more complex solution, might involve bringing in a dedicated tool to manager our configuration (Internal discussions have considered using pydantic settings for instance), but that might be considered out of scope for this ticket, especially if we are considering this ticket to be a "short task."

Acceptance Criteria

  • The SmartSim Config class is broken down into several smaller classes
  • Developers can still access all relevant information via the global CONFIG instance
  • Application and testing code is updated as needed to adapt to the new API. Changes pass static analysis and the test suite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: python Issues related to the Python Client good first issue Issue that is ideal for first-time contributors short task Issues that can be completed and reviewed quickly type: feature Issues that include feature request or feature idea type: refactor Issues focused on refactoring existing code
Projects
None yet
Development

No branches or pull requests

2 participants