Copilot commented on code in PR #16895:
URL: https://github.com/apache/iotdb/pull/16895#discussion_r2609398542
##########
iotdb-core/ainode/iotdb/ainode/core/model/model_storage.py:
##########
@@ -481,41 +474,43 @@ def delete_model(self, model_id: str) -> None:
model_info = category_dict[model_id]
category_value = cat_value
break
-
if not model_info:
logger.warning(f"Model {model_id} does not exist, cannot
delete")
- return
-
+ raise ModelNotExistException(model_id)
if model_info.category == ModelCategory.BUILTIN:
- raise BuiltInModelDeletionError(model_id)
+ logger.warning(f"Model {model_id} is builtin, cannot delete")
+ raise BuiltInModelDeletionException(model_id)
model_info.state = ModelStates.DROPPING
model_path = os.path.join(
self._models_dir, model_info.category.value, model_id
)
- if model_path.exists():
+ if os.path.exists(model_path):
try:
shutil.rmtree(model_path)
- logger.info(f"Deleted model directory: {model_path}")
+ logger.info(f"Model directory is deleted: {model_path}")
except Exception as e:
logger.error(f"Failed to delete model directory
{model_path}: {e}")
- raise
-
- if category_value and model_id in self._models[category_value]:
- del self._models[category_value][model_id]
- logger.info(f"Model {model_id} has been removed from storage")
-
- return
+ model_info.state = (
+ ModelStates.ACTIVE
+ ) # Revert state update on failure
+ raise e
Review Comment:
The model deletion logic has a potential issue. If the model directory does
not exist (line 487 condition is false), the state will remain as DROPPING and
the model entry will be deleted from the dictionary (line 497). This could
leave the system in an inconsistent state if the model directory never existed.
Consider either raising an exception or logging this as a warning before
deletion, or ensure the state is properly handled even when the directory
doesn't exist.
```suggestion
raise e
else:
logger.warning(f"Model directory {model_path} does not exist
when deleting model {model_id}")
```
##########
iotdb-core/ainode/iotdb/ainode/core/model/model_storage.py:
##########
@@ -249,9 +266,9 @@ def register_model(self, model_id: str, uri: str) -> bool:
ensure_init_file(model_dir)
if uri_type == UriType.REPO:
- self._fetch_model_from_hf_repo(parsed_uri, model_dir)
+ _fetch_model_from_hf_repo(parsed_uri, model_dir)
Review Comment:
The parse_uri_type function only accepts URIs starting with "file://" and
raises an exception for all other URIs, but the register_model method (line
268) still checks for UriType.REPO and calls _fetch_model_from_hf_repo. This
means the REPO code path is unreachable because parse_uri_type will raise
InvalidModelUriException before reaching line 268. Either update parse_uri_type
to support "repo://" URIs or remove the unreachable REPO handling code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
