Conversation
lispandfound
commented
Mar 23, 2026
lispandfound
commented
- add check realisation script
- simplify migration script
- fix(check-realisation): correctly store auto response
- refactor(check_script): rename to migrate
- refactor(migrate): move utilities out of migrate script
- feat(migrate): add more refactoring utilities
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the workflow's configuration management by introducing a comprehensive migration script. This script streamlines the process of updating realization configuration files, ensuring they conform to new default structures. It centralizes utility functions, provides interactive prompts for handling schema errors and missing defaults, and adds powerful features for copying and cloning realisations, thereby improving the maintainability and adaptability of the system. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new migration script for realisation configurations and refactors a dictionary merging utility. The refactoring is a good improvement. The new migration script is a valuable addition, but I've found a critical bug that would cause it to crash due to a typo. Additionally, the script's functionality to fill in missing default values, as described in its documentation, appears to be incomplete. I've also included several medium-severity suggestions to improve code clarity, remove dead code, and clean up unused imports.
| ) | ||
|
|
||
| try: | ||
| _ = config.read_from_realisation(realisation) | ||
| except realisations.RealisationParseError: | ||
| if config not in defaults and config != realisations.Seeds: |
There was a problem hiding this comment.
There's a typo here. response.AUTO will raise an AttributeError because response is an instance of the Response enum, not the enum class itself. It should be Response.AUTO.
| ) | |
| try: | |
| _ = config.read_from_realisation(realisation) | |
| except realisations.RealisationParseError: | |
| if config not in defaults and config != realisations.Seeds: | |
| if response in (Response.AUTO, Response.YES): | |
| autofill( | |
| realisation, | |
| default_config, | |
| dry_run=dry_run, | |
| ) |
| error, _ = extract_error( | ||
| config._config_key, config._schema, error |
There was a problem hiding this comment.
Similar to a previous comment, response.AUTO will raise an AttributeError because response is an instance of the Response enum, not the enum class itself. It should be Response.AUTO.
| error, _ = extract_error( | |
| config._config_key, config._schema, error | |
| if response in (Response.AUTO, Response.YES): | |
| trim_keys(realisation, config, extra_keys, dry_run) |
| @@ -0,0 +1,587 @@ | |||
| """Check that realisation can be loaded, if it can't automatically trim extraneous tags and offer to fill in default values.""" | |||
There was a problem hiding this comment.
The script's docstring mentions it will "offer to fill in default values", and the Action enum includes FILL. However, the implementation doesn't seem to handle cases where a schema.SchemaError is due to missing keys. The extract_error function only identifies extraneous keys ("Wrong key(s)"). To fully meet the stated goal, the script should also handle Missing key errors from the schema library, perhaps by prompting the user to fill them from the default configuration.
| return f"Error in {name}: {last_error}", [] | ||
|
|
||
|
|
There was a problem hiding this comment.
The comment ... (keep your existing fuzzy matching logic here) ... is misleading as there is no fuzzy matching logic implemented for suggesting corrections for misspelled keys. This should either be implemented or the comment should be removed to avoid confusion for future maintainers.
| return f"Error in {name}: {last_error}", [] | |
| unknown_key = match.group(1) | |
| return f"Error in {name}: Unknown key '{unknown_key}'", [unknown_key] |
|
|
||
| default_config = defaults.get(config) | ||
| error, extra_keys = extract_error(config._config_key, config._schema, error) | ||
| console.print(error) |
There was a problem hiding this comment.
The special handling for realisations.Seeds in this except block is not immediately obvious. Adding a comment explaining why Seeds is excluded from the "Missing required configuration" error would improve code clarity and maintainability. For example, a comment could explain that Seeds can be generated randomly if missing and doesn't rely on a default configuration in the same way.
| if extra_keys: | ||
| response = auto_response.get((config, Action.TRIM)) or should_trim_keys( | ||
| config, extra_keys | ||
| ) |
There was a problem hiding this comment.
The variable default_config is assigned here but it is not used in the rest of the except block. This appears to be dead code and can be removed.
| @@ -1,5 +1,6 @@ | |||
| """Miscellaneous workflow utilities that couldn't go anywhere else.""" | |||
|
|
|||
| import inspect | |||
There was a problem hiding this comment.
The inspect module is imported but not used in this file. It should be removed to keep the code clean.
| @@ -14,6 +15,7 @@ | |||
| from shapely import Geometry, Polygon, geometry | |||
|
|
|||
| from qcore import coordinates | |||
There was a problem hiding this comment.
The defaults module is imported but not used in this file. It should be removed to keep the code clean.
There was a problem hiding this comment.
Pull request overview
Adds a new migrate utility to help update/repair realisation JSON files against a chosen defaults version, and refactors deep-merge logic into a shared utility used by defaults loading and migration helpers.
Changes:
- Introduces
workflow/scripts/migrate.pywith commands to migrate, copy config blocks, and clone realisations. - Moves deep dictionary merge logic into
workflow.utils.merge_dictionaries(). - Updates
workflow/defaults.pyto use the shared merge utility.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
workflow/utils.py |
Adds merge_dictionaries() utility (but also introduces unused imports / circular import risk). |
workflow/scripts/migrate.py |
New Typer CLI for migration/copy/clone workflows; contains several runtime and CLI-behavior bugs to address. |
workflow/defaults.py |
Refactors defaults merging to use utils.merge_dictionaries(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from qcore import cli | ||
| from workflow import realisations, utils | ||
| from workflow.defaults import DefaultsVersion | ||
| from workflow.realisations import RealisationMetadata, Seeds |
There was a problem hiding this comment.
RealisationMetadata is imported but never used (the code references it via realisations.RealisationMetadata). This will fail linting (ruff F401); please remove the unused import.
| from workflow.realisations import RealisationMetadata, Seeds | |
| from workflow.realisations import Seeds |
| if dry_run: | ||
| console.print( | ||
| f"DRY RUN: Would merge with {config.__class__.__name__} defaults in {realisation}" | ||
| ) | ||
| else: | ||
| config.write_to_realisation(realisation) | ||
|
|
There was a problem hiding this comment.
autofill() claims it will “merge with defaults”, but config.write_to_realisation() overwrites the entire config block for that _config_key (it does not deep-merge with the existing values). If the goal is to fill missing keys while preserving existing user values, load the current config dict, merge defaults into it appropriately, and write the merged result back.
| last_error = e.autos[-1] if e.autos else str(e) | ||
| extraneous_keys = [] | ||
| assert isinstance(last_error, str) | ||
| if "Wrong keys" in last_error: | ||
| extraneous_keys = re.findall(r"'(.*?)'", last_error.split(" in {")[0]) | ||
| error_msg = f"Extraneous keys found: [red]{', '.join(extraneous_keys)}[/red]" | ||
| return f"Error in {name}: {error_msg}", extraneous_keys | ||
|
|
||
| if match := re.match(r"^Wrong key '(.*?)'", last_error): | ||
| unknown_key = match.group(1) | ||
| return f"Error in {name}: Unknown key '{unknown_key}'", [unknown_key] | ||
|
|
||
| return f"Error in {name}: {last_error}", [] | ||
|
|
||
|
|
There was a problem hiding this comment.
extract_error() can crash on non-string entries because of assert isinstance(last_error, str). Schema errors often carry structured context in autos; please handle non-string values defensively (e.g., last_error = str(last_error)) instead of asserting. Also, the placeholder “keep your existing fuzzy matching logic here” and the unused keys/schema variables should be removed or implemented to avoid dead/incomplete code paths.
| error, _ = extract_error( | ||
| config._config_key, config._schema, error | ||
| ) |
There was a problem hiding this comment.
Same enum bug as above: response.AUTO / response.YES will raise AttributeError because response is an instance. Use Response.AUTO / Response.YES.
| console.print(f"[red]Schema error for {realisation}[/red]") | ||
|
|
||
| default_config = defaults.get(config) | ||
| error, extra_keys = extract_error(config._config_key, config._schema, error) | ||
| console.print(error) |
There was a problem hiding this comment.
This branch never reports missing configs: earlier in the loop you continue when config has no defaults, so config not in defaults will be false here. Also, {config.__class__.__name__} will print type because config is itself a class. Consider reporting missing configs unconditionally (except seeds) and use config.__name__ for the class name.
| """ | ||
| with open(realisation_template) as f: | ||
| template = json.load(f) | ||
|
|
||
| configs = configs or list(template) |
There was a problem hiding this comment.
Docstring references “Has no effect when combined with dry run”, but copy() has no dry_run option. Please remove/adjust this text or add the corresponding parameter/behavior.
| realisations[event].add(realisation_number) | ||
|
|
||
| for event, existing_realisations in realisations.items(): |
There was a problem hiding this comment.
Path.is_dir is a method; using realisation.is_dir without calling it is always truthy and defeats the directory check. This should be realisation.is_dir().
| ) | ||
|
|
There was a problem hiding this comment.
default_config is assigned here but never used, which will be flagged by ruff (F841). Please remove the assignment or use it (e.g., to offer a fill/merge action when resolving schema errors).
| @@ -1,5 +1,6 @@ | |||
| """Miscellaneous workflow utilities that couldn't go anywhere else.""" | |||
|
|
|||
| import inspect | |||
There was a problem hiding this comment.
inspect is imported but not used in this module. This will fail linting (ruff F401); please remove the import or use it.
| import inspect |
| from shapely import Geometry, Polygon, geometry | ||
|
|
||
| from qcore import coordinates | ||
| from workflow import defaults |
There was a problem hiding this comment.
from workflow import defaults is unused here and also introduces a circular import with workflow/defaults.py (which now imports workflow.utils). Please remove this import (or refactor to avoid the cycle if it becomes needed later).
| from workflow import defaults |
