Add MVP of one-shot implementation #378
Conversation
ppinchuk
added
enhancement
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 41 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
pyproject.toml:50
compass.utilities.ionow importsyamlandtomlat runtime, but neitherPyYAMLnor atomlpackage is listed in the project dependencies. This will raiseImportErrorin environments where they aren't installed transitively. Add the needed dependencies (or switch TOML loading to the stdlibtomllibfor Python 3.12+), and consider gating YAML/TOML support behind optional extras if you don't want them in the core install set.
| logger.debug("Loading query templates from cache at %s", cache_fp) | ||
| cache = json.loads(cache_fp.read_text(encoding="utf-8")) | ||
| if identifier.casefold() not in qt: | ||
| logger.debug( | ||
| "Adding query templates for %r to cache at %s", | ||
| identifier, | ||
| cache_fp, | ||
| ) | ||
| cache[identifier.casefold()] = { | ||
| "templates": qt, | ||
| "sha256": hashlib.sha256(str(schema).encode()).hexdigest(), | ||
| } | ||
| cache_fp.write_text(json.dumps(cache, indent=4), encoding="utf-8") | ||
| return | ||
|
|
||
| potential_qt = qt[identifier.casefold()] | ||
| m = hashlib.sha256() |
There was a problem hiding this comment.
_qt_to_cache is using the qt list (query templates) where it should be using the cache dict loaded from disk. As written, if identifier.casefold() not in qt: will almost always be true, and potential_qt = qt[identifier.casefold()] will then crash because qt is a list, not a dict. Use cache for membership/indexing and only use qt as the templates payload being stored.
| multiple=True, | ||
| help="One-shot plugin configuration to add to COMPASS before processing", | ||
| ) | ||
| def process(config, verbose, no_progress, plugin): | ||
| """Download and extract ordinances for a list of jurisdictions""" | ||
| config = load_config(config) | ||
|
|
||
| for one_shot_plugin_config in plugin: | ||
| create_schema_based_one_shot_extraction_plugin( | ||
| config=one_shot_plugin_config, tech=config["tech"] |
There was a problem hiding this comment.
The CLI option --plugin/-p is declared as multiple=True, but each provided plugin config is registered with the same identifier tech=config["tech"]. If more than one --plugin is provided, register_plugin will raise because identifiers must be unique. Either make the option non-multiple, or allow each plugin config to supply its own unique identifier and pass that through here.
| multiple=True, | |
| help="One-shot plugin configuration to add to COMPASS before processing", | |
| ) | |
| def process(config, verbose, no_progress, plugin): | |
| """Download and extract ordinances for a list of jurisdictions""" | |
| config = load_config(config) | |
| for one_shot_plugin_config in plugin: | |
| create_schema_based_one_shot_extraction_plugin( | |
| config=one_shot_plugin_config, tech=config["tech"] | |
| help="One-shot plugin configuration to add to COMPASS before processing", | |
| ) | |
| ) | |
| def process(config, verbose, no_progress, plugin): | |
| """Download and extract ordinances for a list of jurisdictions""" | |
| config = load_config(config) | |
| if plugin is not None: | |
| create_schema_based_one_shot_extraction_plugin( | |
| config=plugin, tech=config["tech"] |
| requirements. Keep the response concise and consistent.\ | ||
| """ | ||
| _TEXT_COLLECTION_MAIN_PROMPT = """\ | ||
| Determine wether this text excerpt contains any information relevant to \ |
There was a problem hiding this comment.
Spelling: _TEXT_COLLECTION_MAIN_PROMPT says "Determine wether..."; this should be "whether" to avoid prompting errors and to keep docs/prompt text professional.
| Determine wether this text excerpt contains any information relevant to \ | |
| Determine whether this text excerpt contains any information relevant to \ |
| config_type = ConfigType(config_filepath.name.split(".")[-1]) | ||
| config = config_type.load(config_filepath) | ||
| if resolve_paths: | ||
| return resolve_all_paths(config, config_filepath.parent) | ||
|
|
||
| return config |
There was a problem hiding this comment.
load_config calls ConfigType(...) directly, so an unknown extension (e.g. .txt) will raise a ValueError from Enum construction rather than the documented COMPASSValueError (and the unit test expects COMPASSValueError). Consider catching the Enum error and raising COMPASSValueError with a clear message listing supported extensions.
1 participant
The core fundamentals for using one-shot extraction are in place.
Still TODO (future PR):