Set actionTracingId to tracingId for editableMappingUpdates #8361
Conversation
|
Warning Rate limit exceeded@fm3 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request introduces changes to the migration and annotation versioning tools. A new script Changes
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tools/migration-unified-annotation-versioning/find_mapping_tracing_mapping.py (2)
16-46: Add error handling for file operations and make the output file configurable.Consider the following improvements:
- Add error handling for file operations to handle potential I/O errors.
- Make the output file path configurable via command-line arguments.
Apply this diff to implement the suggestions:
def main(): logger.info("Hello from find_mapping_tracing_mapping") setup_logging() parser = argparse.ArgumentParser() parser.add_argument("--src", type=str, help="Source fossildb host and port. Example: localhost:7155", required=True) parser.add_argument("--postgres", help="Postgres connection specifier, default is postgresql://postgres@localhost:5432/webknossos", type=str, default="postgresql://postgres@localhost:5432/webknossos") + parser.add_argument("--output", help="Output file path for the mapping JSON", type=str, default="mapping_tracing_mapping.json") args = parser.parse_args() before = time.time() annotations = read_annotation_list(args) src_stub = connect_to_fossildb(args.src, "source") mappings = {} for annotation in annotations: annotation_id = annotation["_id"] id_mapping_for_annotation = {} for tracing_id, layer_type in annotation["layers"].items(): if layer_type == 'Volume': try: editable_mapping_id = get_editable_mapping_id(src_stub, tracing_id, layer_type) if editable_mapping_id is not None: id_mapping_for_annotation[editable_mapping_id] = tracing_id except Exception as e: logger.info(f"exception while checking layer {tracing_id} of {annotation_id}: {e}") if id_mapping_for_annotation: mappings[annotation_id] = id_mapping_for_annotation - outfile_name = "mapping_tracing_mapping.json" - logger.info(f"Writing mapping to {outfile_name}...") - with open(outfile_name, "wb") as outfile: - outfile.write(msgspec.json.encode(mappings)) + try: + logger.info(f"Writing mapping to {args.output}...") + with open(args.output, "wb") as outfile: + outfile.write(msgspec.json.encode(mappings)) + except IOError as e: + logger.error(f"Failed to write mapping to {args.output}: {e}") + sys.exit(1) - log_since(before, f"Wrote full id mapping to {outfile_name}. Checked {len(annotations)} annotations, wrote {len(mappings)} annotation id mappings.") + log_since(before, f"Wrote full id mapping to {args.output}. Checked {len(annotations)} annotations, wrote {len(mappings)} annotation id mappings.")
70-88: Remove unnecessary f-string prefixes.The SQL queries don't use any f-string interpolation.
Apply this diff to fix the f-string usage:
- cursor.execute(f"SELECT COUNT(*) FROM webknossos.annotations") + cursor.execute("SELECT COUNT(*) FROM webknossos.annotations") - query = f"""SELECT + query = """SELECT a._id, JSON_OBJECT_AGG(al.tracingId, al.typ) AS layers, JSON_OBJECT_AGG(al.tracingId, al.name) AS layerNames FROM webknossos.annotation_layers al JOIN webknossos.annotations a on al._annotation = a._id GROUP BY a._id """🧰 Tools
🪛 Ruff (0.8.2)
74-74: f-string without any placeholders
Remove extraneous
fprefix(F541)
77-84: f-string without any placeholders
Remove extraneous
fprefix(F541)
tools/migration-unified-annotation-versioning/repair_editable_mapping_updates.py (4)
13-30: Simplify dictionary key iteration.The
.keys()method call is unnecessary when iterating over dictionary keys.Apply this diff to simplify the code:
- for annotation_id in id_mapping.keys(): + for annotation_id in id_mapping:🧰 Tools
🪛 Ruff (0.8.2)
27-27: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
33-66: Consider breaking down the function for better maintainability.The function has multiple responsibilities:
- Processing updates in batches
- Updating the
actionTracingId- Managing the put buffer
Consider extracting the update processing logic into a separate function.
Apply this diff to refactor the code:
+def process_update_group(update_group: dict, id_mapping_for_annotation: dict) -> Tuple[bool, int]: + group_changed = False + changed_update_count = 0 + for update in update_group: + if "value" in update: + update_value = update["value"] + if "actionTracingId" in update_value and update_value["actionTracingId"] in id_mapping_for_annotation: + update_value["actionTracingId"] = id_mapping_for_annotation[update_value["actionTracingId"]] + group_changed = True + changed_update_count += 1 + return group_changed, changed_update_count + def repair_updates_of_annotation(stub, annotation_id, id_mapping_for_annotation, json_encoder, json_decoder): get_batch_size = 100 # in update groups put_buffer_size = 100 # in update groups before = time.time() put_buffer = [] changed_update_count = 0 newest_version = get_newest_version(stub, annotation_id, "annotationUpdates") if newest_version > 10000: logger.info(f"Newest version of {annotation_id} is {newest_version}. This may take some time...") for batch_start, batch_end in reversed(list(batch_range(newest_version + 1, get_batch_size))): update_groups_batch = get_update_batch(stub, annotation_id, batch_start, batch_end - 1) for version, update_group_bytes in update_groups_batch: update_group = json_decoder.decode(update_group_bytes) - group_changed = False - for update in update_group: - if "value" in update: - update_value = update["value"] - if "actionTracingId" in update_value and update_value["actionTracingId"] in id_mapping_for_annotation: - update_value["actionTracingId"] = id_mapping_for_annotation[update_value["actionTracingId"]] - group_changed = True - changed_update_count += 1 + group_changed, group_changed_count = process_update_group(update_group, id_mapping_for_annotation) + changed_update_count += group_changed_count if group_changed: versioned_key_value_pair = proto.VersionedKeyValuePairProto() versioned_key_value_pair.key = annotation_id versioned_key_value_pair.version = version versioned_key_value_pair.value = json_encoder.encode(update_group) put_buffer.append(versioned_key_value_pair) if len(put_buffer) >= put_buffer_size: put_multiple_keys_versions(stub, "annotationUpdates", put_buffer) put_buffer = [] if len(put_buffer) > 0: put_multiple_keys_versions(stub, "annotationUpdates", put_buffer) log_since(before, f"Repaired {changed_update_count} updates of annotation {annotation_id},")
33-66: Consider parameterizing batch sizes.The batch sizes are currently hardcoded. Consider making them configurable via command line arguments for better flexibility in different environments.
def main(): parser = argparse.ArgumentParser() parser.add_argument("--fossil", type=str, help="Fossildb host and port. Example: localhost:7155", required=True) parser.add_argument("--id_mapping", type=str, help="json file containing the id mapping determined by find_mapping_tracing_mapping.py", required=True) + parser.add_argument("--get_batch_size", type=int, help="Batch size for get operations", default=100) + parser.add_argument("--put_batch_size", type=int, help="Batch size for put operations", default=100) args = parser.parse_args() def repair_updates_of_annotation(stub, annotation_id, id_mapping_for_annotation, json_encoder, json_decoder): - get_batch_size = 100 # in update groups - put_buffer_size = 100 # in update groups + get_batch_size = args.get_batch_size # in update groups + put_buffer_size = args.put_batch_size # in update groups
84-90: Consider using consistent error handling.Other functions use
assert_grpc_success, but this one only checks the success flag. Consider using the same error handling pattern for consistency.def get_newest_version(stub, tracing_id: str, collection: str) -> int: getReply = stub.Get( proto.GetRequest(collection=collection, key=tracing_id, mayBeEmpty=True) ) + assert_grpc_success(getReply) if getReply.success: return getReply.actualVersion return 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tools/migration-unified-annotation-versioning/.gitignore(1 hunks)tools/migration-unified-annotation-versioning/find_mapping_tracing_mapping.py(1 hunks)tools/migration-unified-annotation-versioning/migration.py(5 hunks)tools/migration-unified-annotation-versioning/repair_editable_mapping_updates.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tools/migration-unified-annotation-versioning/.gitignore
🧰 Additional context used
🪛 Ruff (0.8.2)
tools/migration-unified-annotation-versioning/find_mapping_tracing_mapping.py
74-74: f-string without any placeholders
Remove extraneous f prefix
(F541)
77-84: f-string without any placeholders
Remove extraneous f prefix
(F541)
tools/migration-unified-annotation-versioning/repair_editable_mapping_updates.py
27-27: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (12)
tools/migration-unified-annotation-versioning/find_mapping_tracing_mapping.py (2)
49-54: LGTM!The function is well-implemented with proper error handling and type hints.
57-67: LGTM!The function is well-implemented with proper type hints and handles all edge cases correctly.
tools/migration-unified-annotation-versioning/repair_editable_mapping_updates.py (6)
69-71: LGTM!The function is well-implemented with proper type hints and error handling.
74-81: LGTM!The function is well-implemented with proper type hints and correctly handles the order of versions and values.
84-90: LGTM!The function is well-implemented with proper type hints and handles empty responses correctly.
13-30: LGTM! Well-structured main function.The function properly handles command line arguments, sets up logging, and uses context managers for file handling.
🧰 Tools
🪛 Ruff (0.8.2)
27-27: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
69-71: LGTM! Proper error handling.The function correctly handles gRPC calls and uses assertions to verify success.
74-81: LGTM! Well-implemented batch retrieval.The function correctly handles gRPC calls, error checking, and maintains chronological order.
tools/migration-unified-annotation-versioning/migration.py (4)
108-108: LGTM!The method signature change improves clarity by explicitly separating
tracing_idandtracing_or_mapping_idparameters.
242-242: LGTM!The change correctly sets
actionTracingIdtotracing_idfor both volume layers and their corresponding mappings, as explained in the comment.
Line range hint
108-128: LGTM! Improved method signature.The updated signature correctly separates
tracing_idandtracing_or_mapping_id, aligning with the PR objectives.
242-242: LGTM! Bug fix for actionTracingId.The code now correctly uses
tracing_idforactionTracingId, even for mappings, fixing the reported bug.
tools/migration-unified-annotation-versioning/repair_editable_mapping_updates.py
Outdated
Show resolved
Hide resolved
Example file for id mapping (annotationId: → (mappingId → tracingId)):