fix quant config issue & ui error issues#7
Conversation
Root Cause: - NiceGUI's ui.number component returns float values by default - The tokenizer's enable_truncation() expects max_length as integer - This caused: TypeError: argument 'max_length': 'float' object cannot be interpreted as an integer Changes: - Convert max_seq_length to int in QuantizationConfig.__init__ - Also convert max_calib_samples to int for consistency Impact: - Tokenization will now succeed with proper integer max_length - Both calibration parameters are guaranteed to be integers Error path before fix: UI (float) → QuantizationConfig (float) → oneshot (float) → tokenizer.enable_truncation(max_length=1024.0) → TypeError After fix: UI (float) → QuantizationConfig (int) → oneshot (int) → tokenizer.enable_truncation(max_length=1024) → Success
Issue:
- NiceGUI's ui.number component returns float values
- group_size from ui.number('Group Size', ...) returns float
- w_bit from ui.select should be int but converting for safety
Changes:
- Convert w_bit to int (line 42)
- Convert group_size to int (line 43)
All numeric parameters in QuantizationConfig now guaranteed to be integers:
✓ max_calib_samples (int)
✓ max_seq_length (int)
✓ w_bit (int)
✓ group_size (int)
This prevents potential type errors downstream in quantization libraries
that expect integer parameters.
Changes: - Dataset Split: Changed from text input to editable select Options: ['train', 'test', 'validation', 'train[:512]', 'test[:512]'] Added clearable prop (field is optional) - Max Calibration Samples: Changed from number input to editable select Options: [128, 256, 512, 1024, 2048] Default: 256 - Max Sequence Length: Changed from number input to editable select Options: [512, 1024, 2048, 4096, 8192] Default: 2048 - Group Size: Changed from number input to editable select Options: [32, 64, 128, 256] Default: 128 Benefits: ✓ Users can pick from common values via dropdown ✓ Users can still type custom values (use-input prop) ✓ new-value-mode=add-unique allows adding typed values ✓ Guided defaults prevent invalid configurations ✓ Better UX than plain number/text inputs All fields remain fully editable while providing sensible defaults.
Root Cause:
- ui.timer(2.0, update_status) calls update_status every 2 seconds
- update_status() showed notifications every time status was
'failed', 'completed', or 'cancelled'
- No check to see if notification was already shown
- Result: 30+ duplicate "Job failed..." notifications
Changes:
- Added last_notified_status state dict to track:
- Last notified status ('failed', 'completed', etc.)
- Last notified message (error text, result, etc.)
- Updated notification logic:
- 'completed': Only notify on first completion
- 'failed': Only notify on first failure OR if error message changes
- 'cancelled': Only notify on first cancellation
- 'idle': Reset notification state (ready for next job)
Benefits:
✓ Each status change notifies exactly once
✓ Different error messages still trigger new notifications
✓ State resets when returning to idle
✓ No more notification spam
✓ User sees clean, non-repetitive status updates
Before: 30+ "Job failed..." notifications every 2 seconds
After: 1 "Job failed..." notification per failure
|
Automated review 🤖 Summary of Changes Key Changes & Positives
Potential Issues & Recommendations
Language/Framework Checks
Security & Privacy Build/CI & Ops Tests Approval Recommendation
|
There was a problem hiding this comment.
Pull Request Overview
This PR improves the configuration UI by replacing numeric input fields with dropdown selects for better user experience, and prevents notification spam in the job monitor by tracking notification state.
- Replaced manual input fields with dropdown selects for common configuration values
- Added input validation by converting string inputs to integers in the QuantizationConfig constructor
- Implemented notification deduplication to prevent repeated UI notifications for the same job status
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/msquant/core/quantizer/config.py | Added int() conversions for numeric parameters to handle values from UI select components |
| src/msquant/app/pages/configure.py | Replaced numeric inputs with select dropdowns for calibration and AWQ settings |
| src/msquant/app/pages/monitor.py | Added notification state tracking to prevent duplicate notifications on status updates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.max_calib_samples = int(max_calib_samples) | ||
| self.max_seq_length = int(max_seq_length) |
There was a problem hiding this comment.
Converting parameters to int() without validation could raise ValueError for invalid input. Consider adding try-except blocks with clear error messages, or validate the input before conversion to provide better error handling for API consumers.
| self.w_bit = w_bit | ||
| self.group_size = group_size | ||
| self.w_bit = int(w_bit) | ||
| self.group_size = int(group_size) |
There was a problem hiding this comment.
Converting parameters to int() without validation could raise ValueError for invalid input. Consider adding try-except blocks with clear error messages, or validate the input before conversion to provide better error handling for API consumers.
| self.group_size = int(group_size) | |
| try: | |
| self.group_size = int(group_size) | |
| except (ValueError, TypeError): | |
| raise ValueError(f"group_size must be an integer, got {group_size!r}") |
| ui.select( | ||
| ['train', 'test', 'validation', 'train[:512]', 'test[:512]'], | ||
| value='train', | ||
| label='Dataset Split' |
There was a problem hiding this comment.
The select component allows custom input via new-value-mode=add-unique, which means users can enter non-integer values. Combined with the int() conversion in config.py line 38-39, this could cause runtime errors when processing custom dataset split strings like 'train[:512]'. Consider validating the input or documenting that dataset split values are strings and should not be converted to integers.
| label='Dataset Split' | |
| label='Dataset Split (e.g., train, test, validation, or custom like train[:512])' |
| ui.select( | ||
| [128, 256, 512, 1024, 2048], | ||
| value=256, | ||
| label='Max Calibration Samples' | ||
| ).classes('flex-1').bind_value(form_data, 'max_calib_samples').props('use-input new-value-mode=add-unique') | ||
| ui.select( | ||
| [512, 1024, 2048, 4096, 8192], | ||
| value=2048, | ||
| label='Max Sequence Length' | ||
| ).classes('flex-1').bind_value(form_data, 'max_seq_length').props('use-input new-value-mode=add-unique') |
There was a problem hiding this comment.
The new-value-mode=add-unique prop allows users to enter arbitrary string values. If a user enters a non-numeric value, the int() conversion in config.py (lines 38-39) will raise an uncaught ValueError. Consider adding input validation in the UI or in the start_quantization() error handler to provide user-friendly error messages.
| ui.select( | ||
| [32, 64, 128, 256], | ||
| value=128, | ||
| label='Group Size' | ||
| ).classes('flex-1').bind_value(form_data, 'group_size').props('use-input new-value-mode=add-unique') |
There was a problem hiding this comment.
The new-value-mode=add-unique prop allows users to enter arbitrary string values. If a user enters a non-numeric value, the int() conversion in config.py (line 43) will raise an uncaught ValueError. Consider adding input validation in the UI or in the start_quantization() error handler to provide user-friendly error messages.
Type Annotation Fix: - Added explicit type hint for last_notified_status dict Type: dict[str, str | None] - Fixes Pyright errors about assigning strings to None type - Properly declares that dict values can be str or None Input Validation Fix: - Wrapped all int() conversions in try-except blocks - Added validation for positive values where appropriate - Provides clear error messages for invalid inputs Validation Details: - max_calib_samples: Must be positive integer - max_seq_length: Must be positive integer - w_bit: Must be integer (range validated in validate()) - group_size: Must be positive integer Error Messages: - ValueError with descriptive message showing invalid value - Catches both ValueError (bad string) and TypeError (None/invalid type) - Original exception chained for debugging Benefits: ✓ Prevents runtime errors from invalid user input ✓ UI allows custom values via editable combo boxes ✓ Clear error messages guide users to fix issues ✓ Type checker passes without errors ✓ Robust handling of edge cases (None, invalid strings, etc.) Example error: Input: max_seq_length="abc" Error: Invalid max_seq_length value 'abc': must be a positive integer
|
Automated review 🤖 Summary of Changes Key Changes & Positives
Potential Issues & Recommendations
Language/Framework Checks
Security & Privacy
Build/CI & Ops
Tests
Approval Recommendation
|
No description provided.