Improve portability and inference workflow - #1
Conversation
📝 WalkthroughWalkthroughScripts refactored from hardcoded, top-level execution to CLI-driven programs with structured functions. Dataset creation, model inference, and training now accept configurable arguments, improved error handling, deterministic seeding, and resource cleanup patterns. Dependencies added to requirements.txt to support functionality. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
create_dataset.py (2)
53-100: Consider usingtry/finallyfor MediaPipe cleanup.If an exception occurs during dataset building (e.g., a corrupt image causing an unexpected error),
hands.close()on line 99 would be skipped. Using atry/finallyblock ensures cleanup even on failure.♻️ Proposed refactor for safer cleanup
def build_dataset(data_dir, preview): mp_hands = mp.solutions.hands hands = mp_hands.Hands(static_image_mode=True, min_detection_confidence=0.3) data = [] labels = [] - for class_dir in sorted(data_dir.iterdir()): - # ... existing loop code ... - - hands.close() - return data, labels + try: + for class_dir in sorted(data_dir.iterdir()): + # ... existing loop code ... + return data, labels + finally: + hands.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@create_dataset.py` around lines 53 - 100, The build_dataset function may skip calling hands.close() if an exception occurs; wrap the processing loop in a try/finally: create the MediaPipe Hands instance (hands = mp_hands.Hands(...)) as you do now, then put the image-processing loop and all logic inside a try block and call hands.close() inside the finally block so cleanup always runs; ensure hands is defined before the try so the finally can reference it safely.
78-91: Coordinate lists accumulate across multiple hands.If an image contains multiple hands,
x_coordsandy_coordsaccumulate landmarks from all hands before computingmin_x/min_y, which could produce inconsistent feature vectors. For single-hand sign datasets this works, but consider resetting these lists inside thefor hand_landmarksloop if multi-hand images are possible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@create_dataset.py` around lines 78 - 91, x_coords and y_coords are populated outside the per-hand loop so landmarks from multiple hands are mixed before computing min_x/min_y; reset x_coords and y_coords at the start of the for hand_landmarks in results.multi_hand_landmarks loop (and compute min_x/min_y immediately per hand) so data_aux entries for each hand use that hand's local coordinates (refer to variables x_coords, y_coords, data_aux and the for hand_landmarks in results.multi_hand_landmarks loop).train_classifier.py (1)
14-44: Consider wrapping in amain()function for consistency.Unlike
create_dataset.pyandinference_classifier.py, this script executes at module level without amain()function orif __name__ == "__main__"guard. This means:
- Importing this module triggers training
- Functions cannot be reused programmatically
- Pattern is inconsistent with other refactored scripts
♻️ Proposed refactor to match other scripts
DATA_PATH = os.path.join(os.path.dirname(__file__), "data.pickle") MODEL_PATH = os.path.join(os.path.dirname(__file__), "model.p") -if not os.path.exists(DATA_PATH): - raise FileNotFoundError(f"Dataset file '{DATA_PATH}' not found.") - -with open(DATA_PATH, "rb") as file_obj: - data_dict = pickle.load(file_obj) - -# ... rest of training code ... - -with open(MODEL_PATH, "wb") as file_obj: - pickle.dump({"model": model}, file_obj) +def main(): + if not os.path.exists(DATA_PATH): + raise FileNotFoundError(f"Dataset file '{DATA_PATH}' not found.") + + with open(DATA_PATH, "rb") as file_obj: + data_dict = pickle.load(file_obj) + + # ... rest of training code ... + + with open(MODEL_PATH, "wb") as file_obj: + pickle.dump({"model": model}, file_obj) + + +if __name__ == "__main__": + main()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@train_classifier.py` around lines 14 - 44, Move the module-level training code into a new main() (or train_model()) function and add an if __name__ == "__main__": main() guard so importing train_classifier.py doesn't trigger training; specifically, wrap the DATA_PATH existence check, pickle load of data_dict, train_test_split call, model creation (RandomForestClassifier), training (model.fit), prediction/accuracy logic (y_predict, accuracy_score) and the final pickle.dump to MODEL_PATH inside main(), and keep DATA_PATH and MODEL_PATH as module-level constants so other modules can call the new train_model()/main() function programmatically.requirements.txt (1)
1-10: Consider pinning dependency versions for reproducibility.Unpinned dependencies can lead to non-reproducible builds and unexpected breakages when upstream packages release incompatible updates. Consider specifying version constraints (e.g.,
mediapipe>=0.10.0,<0.11or exact pins).Additionally, some dependencies appear unused in the provided code:
keras-facenet,mtcnn,ultralytics— not imported in the reviewed scriptsqrcode[pil]— unrelated to hand-sign recognitionIf these are needed for other parts of the project, that's fine. Otherwise, consider removing unused dependencies to reduce install footprint and potential security surface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@requirements.txt` around lines 1 - 10, The requirements file currently lists unpinned packages which harms reproducibility and includes likely-unused entries; update requirements.txt to pin versions for each dependency used by the project (e.g., add exact or range constraints for mediapipe, numpy, opencv-python, scikit-learn, tensorflow, matplotlib) and remove any truly unused packages (inspect imports/usages for keras-facenet, mtcnn, ultralytics, qrcode[pil] and delete them if not required); ensure any retained packages have appropriate version constraints (e.g., mediapipe >= and < constraints) and run a fresh install + test to confirm the pinned set works.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@inference_classifier.py`:
- Around line 40-42: The pickle.load call that deserializes the model from
model_path (returning model_dict["model"]) is unsafe for untrusted files; update
the code around the deserialization (the function that opens model_path) to
either (1) explicitly validate/trust the source before loading (add a clear
check/flag like require_trusted_model or validate checksum/signature) and log a
security warning when loading external files, or (2) replace pickle with a safer
serialization approach for models (e.g., use joblib or a vetted safe Unpickler),
and document in the function docstring that model files must be trusted; ensure
you reference model_path and the function that returns model_dict["model"] so
reviewers can locate the change.
- Line 11: The LABELS dict uses integer keys but model predictions are stored as
string class names, so int(prediction[0]) will raise a ValueError; fix by either
(A) changing LABELS to use string keys matching class names (e.g., LABELS =
{"A": "A", "C": "C", "L": "L"}) so lookups with prediction[0] succeed, or (B)
remove the unnecessary lookup and use prediction[0] (the predicted class string)
directly where it's consumed — update references to LABELS and any code that
does int(prediction[0]) to align with the chosen approach (look for LABELS and
the variable prediction/prediction[0]).
---
Nitpick comments:
In `@create_dataset.py`:
- Around line 53-100: The build_dataset function may skip calling hands.close()
if an exception occurs; wrap the processing loop in a try/finally: create the
MediaPipe Hands instance (hands = mp_hands.Hands(...)) as you do now, then put
the image-processing loop and all logic inside a try block and call
hands.close() inside the finally block so cleanup always runs; ensure hands is
defined before the try so the finally can reference it safely.
- Around line 78-91: x_coords and y_coords are populated outside the per-hand
loop so landmarks from multiple hands are mixed before computing min_x/min_y;
reset x_coords and y_coords at the start of the for hand_landmarks in
results.multi_hand_landmarks loop (and compute min_x/min_y immediately per hand)
so data_aux entries for each hand use that hand's local coordinates (refer to
variables x_coords, y_coords, data_aux and the for hand_landmarks in
results.multi_hand_landmarks loop).
In `@requirements.txt`:
- Around line 1-10: The requirements file currently lists unpinned packages
which harms reproducibility and includes likely-unused entries; update
requirements.txt to pin versions for each dependency used by the project (e.g.,
add exact or range constraints for mediapipe, numpy, opencv-python,
scikit-learn, tensorflow, matplotlib) and remove any truly unused packages
(inspect imports/usages for keras-facenet, mtcnn, ultralytics, qrcode[pil] and
delete them if not required); ensure any retained packages have appropriate
version constraints (e.g., mediapipe >= and < constraints) and run a fresh
install + test to confirm the pinned set works.
In `@train_classifier.py`:
- Around line 14-44: Move the module-level training code into a new main() (or
train_model()) function and add an if __name__ == "__main__": main() guard so
importing train_classifier.py doesn't trigger training; specifically, wrap the
DATA_PATH existence check, pickle load of data_dict, train_test_split call,
model creation (RandomForestClassifier), training (model.fit),
prediction/accuracy logic (y_predict, accuracy_score) and the final pickle.dump
to MODEL_PATH inside main(), and keep DATA_PATH and MODEL_PATH as module-level
constants so other modules can call the new train_model()/main() function
programmatically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3d18861-15d5-4561-9236-8ac419654d4d
📒 Files selected for processing (4)
create_dataset.pyinference_classifier.pyrequirements.txttrain_classifier.py
| cv2.destroyAllWindows() No newline at end of file | ||
|
|
||
| DEFAULT_MODEL_PATH = os.path.join(os.path.dirname(__file__), "model.p") | ||
| LABELS = {0: "A", 1: "C", 2: "L"} |
There was a problem hiding this comment.
Label type mismatch will cause runtime failure.
The LABELS dict uses integer keys, but create_dataset.py stores labels as strings (directory names like "A", "C", "L"). The model trained on these string labels will predict strings, not integers.
At line 109, int(prediction[0]) will raise ValueError when prediction[0] is a string like "A".
🐛 Proposed fix: Use string keys or remove the lookup
Option 1: Use string keys to match the actual predictions:
-LABELS = {0: "A", 1: "C", 2: "L"}
+LABELS = {"A": "A", "C": "C", "L": "L"} # Or simply use prediction directlyOption 2: Simplify by using the prediction directly (since it's already the class name):
- prediction = model.predict([np.asarray(data_aux)])
- predicted_character = LABELS.get(int(prediction[0]), str(prediction[0]))
+ prediction = model.predict([np.asarray(data_aux)])
+ predicted_character = str(prediction[0])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@inference_classifier.py` at line 11, The LABELS dict uses integer keys but
model predictions are stored as string class names, so int(prediction[0]) will
raise a ValueError; fix by either (A) changing LABELS to use string keys
matching class names (e.g., LABELS = {"A": "A", "C": "C", "L": "L"}) so lookups
with prediction[0] succeed, or (B) remove the unnecessary lookup and use
prediction[0] (the predicted class string) directly where it's consumed — update
references to LABELS and any code that does int(prediction[0]) to align with the
chosen approach (look for LABELS and the variable prediction/prediction[0]).
| with open(model_path, "rb") as file_obj: | ||
| model_dict = pickle.load(file_obj) | ||
| return model_dict["model"] |
There was a problem hiding this comment.
Pickle deserialization security consideration.
Loading pickle files from potentially user-provided paths carries risk of arbitrary code execution if the file is malicious. For this local inference tool loading self-trained models, the risk is low, but worth documenting if the tool might be used with untrusted model files.
🧰 Tools
🪛 Ruff (0.15.6)
[error] 41-41: pickle and modules that wrap it can be unsafe when used to deserialize untrusted data, possible security issue
(S301)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@inference_classifier.py` around lines 40 - 42, The pickle.load call that
deserializes the model from model_path (returning model_dict["model"]) is unsafe
for untrusted files; update the code around the deserialization (the function
that opens model_path) to either (1) explicitly validate/trust the source before
loading (add a clear check/flag like require_trusted_model or validate
checksum/signature) and log a security warning when loading external files, or
(2) replace pickle with a safer serialization approach for models (e.g., use
joblib or a vetted safe Unpickler), and document in the function docstring that
model files must be trusted; ensure you reference model_path and the function
that returns model_dict["model"] so reviewers can locate the change.
addz9015
left a comment
addz9015
left a comment
There was a problem hiding this comment.
PR Review Copilot
Security and code quality review of hand-sign recognition workflow improvements
Posted inline comments: 4
| parser.add_argument( | ||
| "--output", | ||
| default=Path(__file__).resolve().parent / "data.pickle", | ||
| type=Path, |
There was a problem hiding this comment.
[MEDIUM] [quality] The dataset creation script does not handle exceptions that may occur during image processing. Consider adding try-except blocks to handle potential errors.
Suggestion: Add try-except blocks to handle exceptions during image processing.
| model = load_model(args.model) | ||
|
|
||
| camera_index = args.camera_index if args.camera_index is not None else find_camera_index() | ||
| if camera_index is None: |
There was a problem hiding this comment.
[MEDIUM] [quality] The inference classifier script does not validate the model file before loading it. Consider adding a check to ensure the model file exists and is valid.
Suggestion: Add a check to ensure the model file exists and is valid before loading it.
| with open(DATA_PATH, "rb") as file_obj: | ||
| data_dict = pickle.load(file_obj) | ||
|
|
||
| data = np.asarray(data_dict["data"]) |
There was a problem hiding this comment.
[LOW] [quality] The training script uses a fixed random seed for reproducibility. Consider using a command-line argument or environment variable to make the seed configurable.
Suggestion: Make the random seed configurable using a command-line argument or environment variable.
| @@ -0,0 +1,10 @@ | |||
| keras-facenet | |||
There was a problem hiding this comment.
[LOW] [quality] The requirements file does not specify the version of the dependencies. Consider adding version numbers to ensure reproducibility.
Suggestion: Add version numbers to the dependencies in the requirements file.
addz9015
left a comment
addz9015
left a comment
There was a problem hiding this comment.
PR Review Copilot
Security and quality review of hand-sign recognition workflow improvements
Posted inline comments: 4
| parser.add_argument( | ||
| "--output", | ||
| default=Path(__file__).resolve().parent / "data.pickle", | ||
| type=Path, |
There was a problem hiding this comment.
[MEDIUM] [quality] The dataset creation script does not handle exceptions that may occur during image processing. Consider adding try-except blocks to handle potential errors.
Suggestion: Add try-except blocks to handle exceptions during image processing, e.g., try: img = cv2.imread(str(image_path)); except Exception as e: print(f'Error reading image {image_path}: {e}')
| model = load_model(args.model) | ||
|
|
||
| camera_index = args.camera_index if args.camera_index is not None else find_camera_index() | ||
| if camera_index is None: |
There was a problem hiding this comment.
[MEDIUM] [quality] The inference script does not validate the model file before loading it. Consider adding a check to ensure the model file exists and is valid.
Suggestion: Add a check to ensure the model file exists and is valid before loading it, e.g., if not os.path.exists(model_path): raise FileNotFoundError(f'Model file {model_path} not found.')
| with open(DATA_PATH, "rb") as file_obj: | ||
| data_dict = pickle.load(file_obj) | ||
|
|
||
| data = np.asarray(data_dict["data"]) |
There was a problem hiding this comment.
[MEDIUM] [quality] The training script does not handle the case where the dataset file is empty. Consider adding a check to ensure the dataset is not empty before training.
Suggestion: Add a check to ensure the dataset is not empty before training, e.g., if len(data) == 0 or len(labels) == 0: raise ValueError('Dataset is empty. Collect data and create data.pickle before training.')
| @@ -0,0 +1,10 @@ | |||
| keras-facenet | |||
There was a problem hiding this comment.
[LOW] [quality] The requirements file does not specify the version of the dependencies. Consider adding version numbers to ensure reproducibility.
Suggestion: Add version numbers to the dependencies in the requirements file, e.g., numpy==1.20.0
1 participant
Summary
This PR improves the hand-sign recognition workflow by making the dataset and training scripts more portable, and by fixing the live webcam inference setup so it behaves correctly for real-time use.
Changes
Removed hardcoded local paths from the dataset creation flow
Added CLI arguments for dataset input/output configuration
Made dataset preview optional instead of forcing a blocking plot for every image
Updated hand inference to use MediaPipe live-video settings instead of static-image mode
Added automatic camera selection for inference
Improved model loading and camera error handling
Added safer bounding box rendering and resource cleanup
Added dataset existence and empty-data checks before training
Made training reproducible with fixed random seeds
Added the missing Python dependencies to requirements.txt
Why
The current scripts are tied to one local machine and include a few runtime issues that make them harder to reuse:
dataset generation depends on a hardcoded Windows path
inference is configured like a static image pipeline even though it runs on webcam frames
training assumes required files always exist
the repository did not declare its Python dependencies
These changes make the project easier to run on another machine and reduce avoidable runtime failures.
Testing
Verified the updated Python files have no editor-detected errors
Verified the branch was committed and pushed successfully
Impact
This PR should not change the intended project flow, but it makes the existing workflow more reliable and easier to set up for other users.
Summary by CodeRabbit
New Features
Bug Fixes
Chores