Skip to content

Update edit settings to remove unused string list setting #1117

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.

Sign up for GitHub

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

Merged
kumare3 merged 1 commit into from
May 29, 2026
+201 −103
Merged

Update edit settings to remove unused string list setting #1117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions src/flyte/cli/_edit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,7 @@ def settings(cfg: common.CLIConfig, project: str | None, domain: str | None, fro

console = common.Console()

try:
settings = remote.Settings.get_settings_for_edit(project=project, domain=domain)
except Exception as e:
console.print(f"[red]Error fetching settings:[/red] {e}")
raise click.Abort
settings = remote.Settings.get_settings_for_edit(project=project, domain=domain)
Copy link
Copy Markdown
Contributor

@kumare3 kumare3 May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove the Abort?

Copy link
Copy Markdown
Contributor Author

@afbrock afbrock May 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When dogfood was misconfigured i actually got an exception while trying to test, and this re-throwing of an error hides the exception. The exception gets displayed with less noise if there isnt a re-throw.
what it says is that during the handling of the exception, another error was thrown - so you get two stacks, and it isnt clear that they have anything to do with each other.


# -------------------- non-interactive path -----------------------------
if from_file is not None:
Expand Down
89 changes: 22 additions & 67 deletions src/flyte/remote/_settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,6 @@ def __repr__(self) -> str:
_MAP_MERGE_COMMENT = (
"## Map entries merge across scopes — parent entries are shown below and applied unless overridden here."
)
_LIST_ADDITIVE_COMMENT = (
"## List values add across scopes — parent items are shown below and included unless overridden here."
)

# Scope level mapping from proto enum to human-readable strings
_SCOPE_NAMES = {
Expand All @@ -121,12 +118,11 @@ def __repr__(self) -> str:
# ``(dotkey, leaf_kind, field_descriptor)``.
#
# Leaf kinds map to the proto *Setting types:
# "string" → StringSetting
# "int" → Int64Setting
# "bool" → BoolSetting
# "quantity" → QuantitySetting (string-encoded k8s quantity)
# "stringlist" → StringListSetting
# "stringmap" → StringMapSetting
# "string" → StringSetting
# "int" → Int64Setting
# "bool" → BoolSetting
# "quantity" → QuantitySetting (string-encoded k8s quantity)
# "stringmap"→ StringMapSetting


@dataclass(frozen=True)
Expand All @@ -142,12 +138,6 @@ class _LeafIO:
build_kwargs: Callable[[Any], dict[str, Any]]


def _build_list_kwargs(v: Any) -> dict[str, Any]:
if not isinstance(v, (list, tuple)):
raise TypeError(f"expected list for stringlist leaf, got {type(v).__name__}")
return {"list_value": settings_definition_pb2.StringValues(values=[str(x) for x in v])}


def _build_map_kwargs(v: Any) -> dict[str, Any]:
if not isinstance(v, dict):
raise TypeError(f"expected dict for stringmap leaf, got {type(v).__name__}")
Expand Down Expand Up @@ -175,11 +165,6 @@ def _build_map_kwargs(v: Any) -> dict[str, Any]:
lambda leaf: leaf.quantity_value,
lambda v: {"quantity_value": str(v)},
),
"stringlist": _LeafIO(
settings_definition_pb2.StringListSetting,
lambda leaf: list(leaf.list_value.values),
_build_list_kwargs,
),
"stringmap": _LeafIO(
settings_definition_pb2.StringMapSetting,
lambda leaf: dict(leaf.map_value.entries),
Expand All @@ -199,7 +184,6 @@ def _build_map_kwargs(v: Any) -> dict[str, Any]:
"int": "0",
"bool": "false",
"quantity": "''",
"stringlist": "[]",
"stringmap": "{}",
}

Expand Down Expand Up @@ -368,7 +352,6 @@ class Settings(ToJSONMixin):
_version: int = field(default=0, repr=False)
_parent_effective: dict[str, EffectiveSetting] = field(default_factory=dict, repr=False)
_map_entry_origins: dict[str, dict[str, EffectiveSetting]] = field(default_factory=dict, repr=False)
_list_item_origins: dict[str, list[EffectiveSetting]] = field(default_factory=dict, repr=False)

@staticmethod
def available_keys() -> list[str]:
Expand Down Expand Up @@ -415,44 +398,33 @@ def _format_value(value: Any) -> str:
return str(value)

def _render_merge_collection(self, l_setting: LocalSetting) -> list[str]:
"""Render a stringmap or stringlist local override as YAML lines, with
parent-scope items shown as ``#``-commented lines tagged with their
origin scope.
"""Render a stringmap local override as YAML lines, with parent-scope
entries shown as ``#``-commented lines tagged with their origin scope.

When the local container has entries::
When the local map has entries::

## merge/additive comment
## merge comment
{key}:
{local entries...}
# {parent entry} ## defined at {origin}

When the local container is empty, the key line itself is commented so
When the local map is empty, the key line itself is commented so
saving an unedited file does not replace inherited values with ``null``::

## merge/additive comment
## merge comment
# {key}:
# {parent entry} ## defined at {origin}
"""
key = l_setting.key
if _LEAF_TYPES.get(key) == "stringmap":
merge_comment = _MAP_MERGE_COMMENT
local_dict = l_setting.value if isinstance(l_setting.value, dict) else {}
local_items = [(k, f"{k}: {self._format_value(v)}") for k, v in local_dict.items()]
parent_items = [
(k, f"{k}: {self._format_value(eff.value)}", eff.origin)
for k, eff in self._map_entry_origins.get(key, {}).items()
]
else: # stringlist
merge_comment = _LIST_ADDITIVE_COMMENT
local_list = l_setting.value if isinstance(l_setting.value, list) else []
local_items = [(v, f"- {self._format_value(v)}") for v in local_list]
parent_items = [
(eff.value, f"- {self._format_value(eff.value)}", eff.origin)
for eff in self._list_item_origins.get(key, [])
]
local_dict = l_setting.value if isinstance(l_setting.value, dict) else {}
local_items = [(k, f"{k}: {self._format_value(v)}") for k, v in local_dict.items()]
parent_items = [
(k, f"{k}: {self._format_value(eff.value)}", eff.origin)
for k, eff in self._map_entry_origins.get(key, {}).items()
]

local_dedup_keys = {dk for dk, _ in local_items}
lines = [merge_comment]
lines = [_MAP_MERGE_COMMENT]
if local_items:
lines.append(f"{key}:")
lines.extend(f" {repr_str}" for _, repr_str in local_items)
Expand All @@ -477,15 +449,12 @@ def to_yaml_sections(self) -> list[tuple[str, str]]:
local_keys = {s.key for s in self.local_settings}
effective_keys = {s.key for s in self.effective_settings}

# A stringmap or stringlist local setting with no local entries has nothing to override
# A stringmap local setting with no local entries has nothing to override
# — treat it as inherited for display so it appears in the Inherited section, not Local overrides.
empty_local_map_keys = {
s.key
for s in self.local_settings
if (
(_LEAF_TYPES.get(s.key) == "stringmap" and not (isinstance(s.value, dict) and s.value))
or (_LEAF_TYPES.get(s.key) == "stringlist" and not (isinstance(s.value, list) and s.value))
)
if _LEAF_TYPES.get(s.key) == "stringmap" and not (isinstance(s.value, dict) and s.value)
}
display_local_settings = [s for s in self.local_settings if s.key not in empty_local_map_keys]
display_local_keys = local_keys - empty_local_map_keys
Expand All @@ -502,7 +471,7 @@ def _describe(dotkey: str) -> str | None:
d = _describe(l_setting.key)
if d:
lines.append(d)
if _LEAF_TYPES.get(l_setting.key) in ("stringmap", "stringlist"):
if _LEAF_TYPES.get(l_setting.key) == "stringmap":
lines.extend(self._render_merge_collection(l_setting))
else:
lines.append(f"{l_setting.key}: {self._format_value(l_setting.value)}")
Expand All @@ -524,10 +493,6 @@ def _describe(dotkey: str) -> str | None:
lines.append(f"# {setting.key}: ## inherited from {setting.origin}")
for entry_key, entry_val in setting.value.items():
lines.append(f"# {entry_key}: {self._format_value(entry_val)}")
elif _LEAF_TYPES.get(setting.key) == "stringlist" and isinstance(setting.value, list):
lines.append(f"# {setting.key}: ## inherited from {setting.origin}")
for item in setting.value:
lines.append(f"# - {self._format_value(item)}")
else:
lines.append(
f"# {setting.key}: {self._format_value(setting.value)} ## inherited from {setting.origin}"
Expand Down Expand Up @@ -596,7 +561,7 @@ def parse_yaml(yaml_content: str) -> dict[str, Any]:

Uses ``yaml.safe_load``, so all YAML syntax is supported — including
flow collections (``[a, b]``, ``{k: v}``) and block collections — for
the list and map leaves (``labels``, ``annotations``,
the map leaves (``labels``, ``annotations``,
``environment_variables``). Commented lines are ignored (template
entries stay as comments until the user uncomments them).
"""
Expand Down Expand Up @@ -657,7 +622,6 @@ async def get_settings_for_edit(cls, project: str | None = None, domain: str | N
effective: dict[str, EffectiveSetting] = {}
parent_effective: dict[str, EffectiveSetting] = {}
map_entry_origins: dict[str, dict[str, EffectiveSetting]] = {}
list_item_origins: dict[str, list[EffectiveSetting]] = {}
for record in resp.levels:
is_local = (record.key.domain or "") == want_domain and (record.key.project or "") == want_project
origin = _scope_origin_from_key(record.key)
Expand All @@ -671,14 +635,6 @@ async def get_settings_for_edit(cls, project: str | None = None, domain: str | N
entry_map = map_entry_origins.setdefault(dotkey, {})
for entry_key, entry_val in val.items():
entry_map[entry_key] = EffectiveSetting(key=dotkey, value=entry_val, origin=origin)
elif _LEAF_TYPES.get(dotkey) == "stringlist":
if val is UNSET:
list_item_origins.pop(dotkey, None)
elif isinstance(val, list):
item_map = {es.value: es for es in list_item_origins.get(dotkey, [])}
for item in val:
item_map[item] = EffectiveSetting(key=dotkey, value=item, origin=origin)
list_item_origins[dotkey] = list(item_map.values())
effective[dotkey] = EffectiveSetting(key=dotkey, value=val, origin=origin)

# Local settings come from the level whose key equals the requested scope
Expand All @@ -703,7 +659,6 @@ async def get_settings_for_edit(cls, project: str | None = None, domain: str | N
_version=version,
_parent_effective=parent_effective,
_map_entry_origins=map_entry_origins,
_list_item_origins=list_item_origins,
)

@syncify
Expand Down
2 changes: 0 additions & 2 deletions tests/flyte/cli/test_edit_settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,6 @@ def test_from_file_handles_commented_template(tmp_path: Path):
"security.service_account: ml-sa\n"
"\n"
"### Available settings (uncomment and edit to set at this scope)\n"
"## Default queue for task runs\n"
"# run.default_queue: ''\n"
)
result, s, _ = _invoke_from_file(body, tmp_path=tmp_path)
assert result.exit_code == 0, result.output
Expand Down
Loading
Loading