ESMCI / ESMCI/cime

Refactor and API changes

Open
#4,950 3 comments 0 reactions 1 assignee Claimed by @jasonb5 View on GitHub
ty: Discussion
Dominant language
Python
Stars
174
Forks
225
Avg merge
1d 16h
Merged PRs (30d)
14

Description

# CIME Refactor — Master Tracker

Incremental refactor to improve modularity, error handling, and testability
while preserving compatibility with external models (E3SM, CESM, NorESM).

**Core principle: reorganize existing code, don't rewrite it.** Move functions
to better homes, break apart oversized modules, improve error handling. Don't
wrap stdlib (`os.path`, `subprocess`, `os.environ`, `time`) in abstraction
layers — `unittest.mock.patch` already handles those cleanly.

## Sub-issues (slices)

- [ ] #4996 Slice 1 — Foundation: typed exceptions, bootstrap, kill star imports
- [ ] #4997 Slice 2 — Batch: move scheduler/submit logic into `CIME/core/batch/`
- [ ] #4998 Slice 3A — SRCROOT standardization (removes `config_files.xml`)
- [ ] #4999 Slice 3B — Build: move build logic into `CIME/core/build/`
- [ ] #5000 Slice 4 — Case: consolidate, then decompose the god object

Estimated total: 22–24 weeks.

## Compatibility-First Policy

Preserve current usage patterns unless change is absolutely required. Breaking
changes must be:
- explicitly justified,
- accompanied by a migration plan,
- validated with E3SM, CESM, and NorESM representative workflows.

## Target package structure

```
CIME/
├── core/ # Reorganized internals (all implementation lives here)
│ ├── config/ # Bootstrap, SRCROOT, config loading
│ ├── batch/ # Batch/scheduler logic
│ ├── build/ # Build logic
│ ├── status/ # Status tracking
│ ├── locking/ # Lock management
│ ├── exceptions.py # Typed exception hierarchy
│ ├── shell.py # run_cmd, run_cmd_no_fail (from utils.py)
│ ├── logging.py # Logging setup (from utils.py)
│ ├── fileops.py # File helpers (from utils.py)
│ ├── time.py # Time conversion helpers (from utils.py)
│ └── convert.py # Type conversion helpers (from utils.py)
├── utils.py # Thin re-exports only (was 2700 lines)
├── build.py # Thin re-exports only
├── case/ # Thinned, delegates to core/
└── ...
```

Existing import paths remain valid via re-exports until downstream models migrate.

## Migration pattern

For each function/class:
1. Move it into the appropriate `CIME/core/` module.
2. In its original module, leave a thin re-export so external callers (E3SM,
CESM, NorESM) are unaffected.
3. Update internal CIME imports to point at the new `core/` location.
4. Add/update tests against the new location.

Example — moving `run_cmd`:

```python
# CIME/core/shell.py (real code lives here)
def run_cmd(cmd, ...):
...

# CIME/utils.py (thin re-export for external consumers)
from CIME.core.shell import run_cmd # noqa: F401

# CIME/case/case_submit.py (internal — updated)
from CIME.core.shell import run_cmd
```

## What we DO

- Move functions out of bloated modules (`utils.py` at 2700 lines) into focused
modules under `CIME/core/`.
- Consolidate scattered free functions that take an object and mutate its state
back into the class that owns them — especially around `Case`.
- Extract coherent subsystems from `Case` (status tracking, locking, XML
storage) into focused modules.
- Eliminate star imports, global mutable state, and import-time side effects.
- Improve error handling with a typed exception hierarchy.
- Fix test infrastructure so unit tests can run without host model config.

## What we DON'T do

- Don't wrap stdlib behind protocol classes (`os.path`, `subprocess`, `time`,
`os.environ`).
- Don't introduce DI frameworks or service locators.
- Don't rewrite working code for aesthetics.
- Don't break the `Case` API or `build_scripts/` interface.

DI, protocols, and factory functions are used **only** where CIME has genuine
polymorphism (scheduler backends, config loaders).

## Cross-cutting issues addressed in Slice 1

- **Star imports** — `from CIME.XML.standard_module_setup import *` in ~60
files; `from CIME.test_status import *` in ~10 files. Replace with explicit
imports.
- **Global mutable state** — `GLOBAL = {}` in `utils.py:21` used to pass
`SRCROOT` between modules; plus `_CIMECONFIG`, `_TIME_CACHE`, `_ALL_TESTS`.
- **Import-time side effects** — `Servers/__init__.py` runs `shutil.which()`
at import time; `standard_script_setup.py` and `standard_module_setup.py`
modify `sys.path` at import time.
- **Inconsistent error handling** — 5 patterns in use (`expect()`, `raise
CIMEError`, `sys.exit()`, `raise RuntimeError`, `logger.fatal()` +
`sys.exit()`). Standardize on `expect()` / `CIMEError` for library code.
- **Circular imports** — `Case` injects methods from 10 sibling modules at
class body level (case.py:84-102) to work around cycles.
- **Test infrastructure** — `conftest.py` requires host model config and
machine XML; unit tests cannot run standalone.

## Large modules requiring decomposition

| Class / module | File | Methods / funcs | Lines | Slice |
|---|---|---:|---:|---|
| `Case` | `case/case.py` | 63 | 2600 | 4 |
| `EnvBatch` | `XML/env_batch.py` | 43 | 1590 | 2 |
| `case/case_st_archive.py` | (procedural) | ~20 | 1395 | 4 |
| `TestScheduler` | `test_scheduler.py` | 28 | 1340 | 4 |
| `build.py` | (procedural) | — | 1350 | 3B |
| `GenericXML` | `XML/generic_xml.py` | 41 | 700 | 4 (later) |
| `NamelistGenerator` | `nmlgen.py` | 36 | 870 | 4 (later) |
| `EnvMachSpecific` | `XML/env_mach_specific.py` | 35 | 770 | 4 (later) |
| `case/check_input_data.py` | (procedural) | ~12 | 693 | 4 |
| `baselines/performance.py` | (procedural) | ~12 | 612 | 4 |
| `hist_utils.py` | (procedural) | ~7 | 836 | 4 |

## Success criteria

1. **Compatibility** — external models work without modification (or with
documented migration).
2. **Testability** — 80%+ coverage for reorganized code.
3. **Maintainability** — no module over ~500 lines; clear boundaries.
4. **Validation** — each slice passes E3SM, CESM, NorESM representative
workflows; no perf regression.

## Related issues

- #4845 — CLI & packaging update (single-entrypoint feature, complementary)
- #4956 — Standardize CIME submodule conventions (subsumed into Slice 3A)
- #3923 — Standalone-checkout detection (subsumed into Slice 3A)
- #4792 — Refactor `config_archive.xml` / `case_st_archive.py` (Slice 4)
- #4837 — Sharedlibs hard-coding (Slice 3B)
- #4936 — `CONTRIBUTING.md` test instructions wrong (Slice 1, conftest fix)
- #4528 — Earlier refactor RFC, superseded and closed in favor of this tracker

## How to contribute

Each slice has its own issue with detailed tasks. Pick a slice, claim the
issue, open a draft PR against `master`. The reorganize-don't-rewrite rule
applies to every PR — review checklist is in each slice issue.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.