Skip to content

Fix quantization param overrides in task grid#7

Open
Sbboss wants to merge 2 commits intonovitalabs:mainfrom
Sbboss:fix/quant-params-override
Open

Fix quantization param overrides in task grid#7
Sbboss wants to merge 2 commits intonovitalabs:mainfrom
Sbboss:fix/quant-params-override

Conversation

@Sbboss
Copy link

@Sbboss Sbboss commented Jan 11, 2026

Summary

Changes

  • Promote quantization fields from parameters into __quant__* for grid expansion.
  • Add tests to verify user-specified kv-cache dtype overrides defaults.

Testing

  • python tests/test_quant_param_override.py

Checklist

  • I have added/updated tests where needed.
  • I have run the relevant tests.
  • I have updated documentation if required.

Notes

@k-l-lambda
Copy link
Collaborator

Code Review

The core fix looks correct and tests are valuable. A few suggestions before merging:

1. Precedence Rule Clarification

When both __quant__* and alias keys (e.g., kv-cache-dtype) are provided simultaneously, the current behavior is that the alias wins. Please:

  • Document this precedence rule explicitly
  • Add a test case for this scenario

2. Test Robustness

test_prepare_runtime_parameters_respects_override uses grid[0] which assumes a specific ordering. Consider:

  • Iterating through all grid entries, or
  • Using set-based assertions to avoid flakiness

3. Code Style

  • The test file uses tabs for indentation while the project appears to use spaces
  • Consider using standard imports instead of sys.path manipulation

4. PR Template Issue

The .github/PULL_REQUEST_TEMPLATE.md contains this PR's actual content rather than a generic template with placeholders. Should be a separate commit or removed.


Review assisted by GPT-5.2

@Sbboss
Copy link
Author

Sbboss commented Jan 12, 2026

Thanks for the review.

I have made the pertinent changes, please check once.

@Sbboss
Copy link
Author

Sbboss commented Jan 24, 2026

Hey @k-l-lambda ,
Reminder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants