Skip to content

[Console] Install console runtime in dev sync#199

Closed
suraj-ranganath wants to merge 17 commits into
developfrom
fix/fresh-uv-console
Closed

[Console] Install console runtime in dev sync#199
suraj-ranganath wants to merge 17 commits into
developfrom
fix/fresh-uv-console

Conversation

@suraj-ranganath

@suraj-ranganath suraj-ranganath commented Jun 12, 2026

Copy link
Copy Markdown
Member

Stacked on #190; do not merge before the earlier stack lands. Add GUI and console runtime dependencies to the default development group so a fresh source checkout can run uv run eegprep-console --full without a separate extra sync. Also addresses review-found stacked regressions in pop_newset, session validation, and long-task logging.

Fixes #192

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Claude finished @suraj-ranganath's task in 6m 5s —— View job


Code review — #199 [Console] Install console runtime in dev sync

  • Overall assessment: Needs changes (one blocking GUI regression).
  • Highest-risk area: pop_newset GUI "Edit description" handling.
  • Merge recommendation: Needs changes.

This PR is stacked on #190, so the diff against develop includes the entire stack (66 files). The stated goal — adding the GUI/console runtime deps to the default dev group — is implemented correctly (pyproject.toml:88-93 matches the existing gui/console extras, uv.lock regenerated, docs updated). The findings below are in the broader stacked changes.

Blocking

pop_newset.py:155-159 — confirming the dialog without editing the description clobbers comments to the string "False". (inline)
editdescription is a QPushButton and there is no comments tag in the spec, so result.get("comments") is always None and the code falls through to result.get("editdescription"). When the user doesn't click "Edit description", the button has no _VALUE_PROPERTY, so _read_widget (qt.py:1247) returns widget.isChecked()False. False is not None, so gui_options["comments"] = str(False) = "False" — every normal "name dataset → OK" flow silently overwrites EEG['comments']. Fix: only treat the result as comments when it's a str (the _edit_text callback only stores a str on accept):

edited = result.get("editdescription")
if isinstance(edited, str):
    gui_options["comments"] = edited

Fix this →

Important

None. (The PR scope is much wider than the #192 dependency change, but that is expected for a stacked PR against develop.)

Nits

long_task.py:58-71Worker.run mutates the shared eegprep logger level from the worker thread. It does setLevel(...) / addHandler / removeHandler / setLevel(old_level) on the global logging.getLogger("eegprep"). The save/restore of old_level is not atomic against re-entrant or concurrent long tasks; in practice only one runs at a time, so impact is low. Consider attaching the handler without changing the shared level.

Test gaps

  • pop_newset.py — no test for the un-edited "Edit description" path. Add a case where the renderer returns editdescription: False and assert comments is not injected (this is exactly the blocking bug above). (blocking-adjacent)
  • session.py apply_workspace_state validation guards are untested. This method is now the single transaction point for every GUI/console workspace mutation. Add direct unit tests asserting it raises on (a) a currentset index > len(alleeg) and (b) an eeg list whose length differs from currentset. A regression dropping these guards would silently desync ALLEEG/CURRENTSET — the exact invariant AGENTS.md calls out. (important)
  • pop_runica_gui_options — no test that a non-runica icatype does not get interrupt injected (the inverse of the covered runica case). (nit)

EEGLAB parity notes

No parity regressions found. Verified as correct improvements:

  • eeg_mne2eeg_epochs.py fixes the previously-swapped icasphere/icaweights assignment and now emits channel-major data (nbchan, pnts, trials), icaweights (n_components, n_channels), icasphere (n_channels, n_channels), icawinv, and an F-order icaact round-trip that the updated tests verify.
  • eeg_eeg2mne.py / eeg_mne2eeg.py replace NamedTemporaryFile(delete=False) (which leaked the .set/.fdt bridge files) with TemporaryDirectory; loads happen before cleanup.
  • session.py/console.py/menu_actions.py apply_workspace_state consolidation preserves 1-based↔0-based CURRENTSET handling and ALLEEG mirroring; add_history("") is a no-op so empty command calls don't pollute history.
  • epoch.py gating "Epoching…" on verbose=='on' keeps default behavior (default is 'on').
  • The CLI eeglabmigrate rename updates all call sites with no compat alias, matching the "no backward compatibility by default" rule.

Checked: correctness/regressions, EEGLAB parity, EEG data-structure compatibility, changed-behavior tests, and AGENTS.md/CLAUDE.md compliance. I could not run pytest/ruff/ty locally (sandbox blocked uv run); CI should confirm lint/type/test.

@suraj-ranganath suraj-ranganath mentioned this pull request Jun 12, 2026
Comment on lines +155 to +159
comments = result.get("comments")
if comments is None:
comments = result.get("editdescription")
if comments is not None:
gui_options["comments"] = str(comments)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Blocking — confirming this dialog without editing the description clobbers comments to the literal string "False".

editdescription is a QPushButton and the dialog spec has no comments tag, so result.get("comments") is always None and the code falls through to result.get("editdescription"). When the user does not click "Edit description", the button has no _VALUE_PROPERTY, so _read_widget (qt.py:1247) falls to widget.isChecked()False. Since False is not None, this sets gui_options["comments"] = str(False) = "False". Every plain "name the dataset and click OK" flow silently overwrites EEG['comments'] with "False".

The _edit_text callback only stores a value (a str) on the button when the user actually edits and accepts, so guard on that:

Suggested change
comments = result.get("comments")
if comments is None:
comments = result.get("editdescription")
if comments is not None:
gui_options["comments"] = str(comments)
edited = result.get("editdescription")
if isinstance(edited, str):
gui_options["comments"] = edited

The new test_pop_newset_gui_description_button_value_updates_comments always passes editdescription: "edited notes" (a str), so it misses this path — please add a case where the result has editdescription: False (un-edited) and assert comments is not added to gui_options/left unchanged.

@suraj-ranganath

Copy link
Copy Markdown
Member Author

🤖 Closing this because #206 now carries the fix/fresh-uv-console commits plus the Fable 5 integration fixes into the advisor-gated epic PR path. Keeping both PRs open would duplicate the same console-runtime dependency change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GUI issue

1 participant