Introduce NicknameSettingList for explicit efit tree cascade#559
Introduce NicknameSettingList for explicit efit tree cascade#559samc24 wants to merge 5 commits into
Conversation
Signed-off-by: Sameer Chaturvedi <sameerc@mit.edu>
There was a problem hiding this comment.
Pull request overview
This PR introduces an explicit EFIT tree “cascade” nickname setting to improve robustness when a preferred EFIT tree is absent, and wires the selected cascade into the MDS connection for future retry enhancements.
Changes:
- Added
NicknameSettingListto try a sequence of tree candidates, selecting the first that successfullyopen_tree()’s. - Extended
resolve_nickname_setting()to accept list cascades and comma-separated string cascades, including mapping registered tokens (e.g."disruption","analysis") to theirNicknameSettingimplementations. - Refactored CMOD
DisruptionNicknameSettingto use the new cascade behavior and added unit tests for cascade resolution/fallback.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
tests/test_nickname_setting.py |
Adds unit tests for cascade parsing, validation, and fallback behavior. |
disruption_py/settings/nickname_setting.py |
Implements NicknameSettingList, extends resolver to accept cascades, refactors CMOD disruption tree selection. |
disruption_py/inout/mds.py |
Stores nickname cascade objects on MDSConnection for follow-up retry logic. |
disruption_py/core/retrieval_manager.py |
Passes EFIT nickname cascade metadata into MDSConnection.add_tree_nickname_funcs(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if isinstance(nickname_setting, list): | ||
| return NicknameSettingList(nickname_setting) |
| class NicknameSettingList(NicknameSetting): | ||
| """ | ||
| Cascade nickname setting. Tries each item in order via ``open_tree``, | ||
| returning the first that opens successfully. Falls back to the next on | ||
| ``mdsExceptions.TreeFOPENR``. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| items : list | ||
| Cascade of tree-name strings or NicknameSetting instances. Tried in | ||
| order; the first item that opens wins. | ||
| """ |
| @staticmethod | ||
| def _resolve_item(item): | ||
| """Validate a cascade item: must be a string or NicknameSetting.""" | ||
| if isinstance(item, NicknameSetting): | ||
| return item | ||
| if isinstance(item, str): | ||
| return item | ||
| raise ValueError( | ||
| "NicknameSettingList items must be str or NicknameSetting, " | ||
| f"got {type(item).__name__}: {item!r}" | ||
| ) |
| def add_tree_nickname_funcs( | ||
| self, | ||
| tree_nickname_funcs: Dict[str, Callable], | ||
| tree_nickname_cascades: Dict[str, Any] = None, | ||
| ): |
| def test_resolve_list_cascade(): | ||
| """A list dispatches to NicknameSettingList.""" | ||
| out = resolve_nickname_setting(["efit21", "analysis"]) | ||
| assert isinstance(out, NicknameSettingList) | ||
|
|
| def test_resolve_dict_value_can_be_cascade(): | ||
| """A NicknameSettingDict value may itself be a cascade list.""" | ||
| out = resolve_nickname_setting({Tokamak.CMOD: ["efit21", "efit18", "analysis"]}) | ||
| assert isinstance(out, NicknameSettingDict) | ||
| inner = out.resolved_nickname_setting_dict[Tokamak.CMOD] | ||
| assert isinstance(inner, NicknameSettingList) | ||
| assert inner.resolved_items == ["efit21", "efit18", "analysis"] |
- Resolve registered keys in list-cascade inputs (e.g. `["disruption", "analysis"]`), matching the existing comma-string behavior. Shared via a new `_map_cascade_item` helper. - `NicknameSettingList` docstring now lists both `TreeFOPENR` and `FetchDataError` as fallback triggers. - `_resolve_item` strips strings and rejects empty / whitespace-only items. - `MDSConnection.add_tree_nickname_funcs`: `tree_nickname_cascades` typed as `Optional[Dict[str, Any]]`. - Tests updated for list-cascade key resolution; added `test_list_empty_string_rejected`. Signed-off-by: Sameer Chaturvedi <sameerc@mit.edu>
|
Good catches, CoPilot
Local pre-push: 28/28 nickname tests pass, ruff/isort/pylint clean. |
yumouwei
left a comment
yumouwei
left a comment
There was a problem hiding this comment.
I tested the new efit_nickname_setting=List and it works. Would it be possible to add the names of the EFIT trees used by each shot to the dataset attributes (e.g. as a dictionary)?
|
I think that will have to first be moved over into the configuration, and then be dumped into a JSON or TOML next to the output -- in any case, that will have to happen in a different PR but yes! absolutely, it's on the to-do list. can you provide some useful snippets for eg testing something that has efit21 and efit18 and analysis, vs only efit21 and analysis, and so forth? we need to understand the implications before merging. |
|
We can do something really simple like this: We can't test this on D3D because of their EFIT tree name allocation for DIS & DISPY, which is really annoying. |
|
Thanks @yumouwei — built your snippet into a runnable validation script (
Assertions (all pass):
Verbose log confirms the fallback: @gtrevisan — re: tagging the selected tree onto the dataset attrs, agreed that belongs in the config→JSON/TOML-next-to-output work, not here. Tracking it for that PR. |
Summary
NicknameSettingList: user-specifiable cascade of efit tree candidates. Tries each viaopen_tree, catchesmdsExceptions.TreeFOPENR(tree missing) andFetchDataError(nested cascade exhausted), returns the first that opens.efit_nickname_setting=["efit21", "efit18", "analysis"]) and comma-separated CLI strings (--efit-tree "efit21,efit18,analysis"). Tokens that match registered keys (e.g."disruption") dispatch to theirNicknameSettingclass inside the cascade.DisruptionNicknameSetting._cmod_nicknameto delegate to the new primitive -- replaces thedisruption_time is Noneproxy with real fallback driven by tree-open errors. D3D path unchanged (its rundb SQL query is real functionality, not just cascade).MDSConnection.tree_nickname_cascadesstores the originatingNicknameSettingListso a follow-up PR can extend retry to data-path-missing errors inside an opened tree. Currently written for top-levelNicknameSettingListonly; nested cases (dict-wrapped,DisruptionNicknameSettinginternals) are not yet surfaced.Behavior change
For
efit_nickname_setting="disruption"on CMOD, the legacy proxydisruption_time is None → return "analysis"is replaced with a realopen_tree("efit18")attempt, falling back to"analysis"onTreeFOPENR.Validated bit-identical output vs
devon two CMOD test shots:efit18, 83 idx points, identical data."analysis"via proxy; new triesefit18, getsTreeFOPENR, falls back to"analysis". Both produce 88 idx points, identical data.Trade-off: one extra failed MDS
open_treeper non-disrupting shot using"disruption", in exchange for a verbose log line documenting the fallback (Nickname cascade for shot N: selected 'analysis' after failed: efit18) and a path that handles unanticipated tree-availability patterns without proxy maintenance.Out of scope (Phase 2 follow-up)
FetchDataErrorwhen a tree opens but a data path inside it is missing.NicknameSettingDictor insideDisruptionNicknameSetting._cmod_nickname) toMDSConnection.tree_nickname_cascades.efit_nickname_settingvalue.Test plan
tests/test_nickname_setting.py(25 tests, pure-Python with a fakedata_conn, no MDS required).make black && ruff check . && isort --check . && pylint(CI run 26377445449).mfews04with shot 1150805012:uv run disruption-py --efit-tree "doesnotexist,analysis" 1150805012— cascade selectsanalysisafterdoesnotexistfails; verified via TRACE log.uv run disruption-py --efit-tree "disruption,analysis" 1150805012— first item resolves toDisruptionNicknameSetting, succeeds; 83 idx points (same as plain"disruption").devon shots 1150805012 (disrupting) and 1150805013 (non-disrupting) under--efit-tree "disruption".