build: update ty to v0.0.20 and align type-checking config#535
build: update ty to v0.0.20 and align type-checking config#535begumcig merged 14 commits intoPrunaAI:mainfrom
Conversation
gsprochette
left a comment
There was a problem hiding this comment.
Thanks a lot for taking care of this. It's almost ready to merge but some changes have conflicts with #520 :
- The
get_model_dependent_hyperparameter_defaultsmethod signature was changed so you can rebase and drop the changes in hqq, hqq_diffusers, llm_compressor and torchao - for sage_attn it's the same, but I missed it in the PR. I fixed that in #540. You can rebase onto this PR and drop the changes in this file as well.
Also 0.0.17 is out so let's use it :)
|
|
||
| return any(isinstance(attr_value, tuple(transformer_and_unet_models)) for attr_value in model.__dict__.values()) | ||
|
|
||
| def get_model_dependent_hyperparameter_defaults( |
There was a problem hiding this comment.
The signature for get_model_dependent_hyperparameter_defaults changed in #520 :
- the smash_config argument is always a
SmashConfigPrefixWrapper - the output type is a
dict[str, Any]where keys are hyperparameter names and values are their associated default value. Unfortunately,TARGET_MODULES_TYPEfits that type so some algorithms haven't been properly updated.
In all cases, these signature changes shouldn't be made for algorithms that return e.g. {"target_modules": some_TARGET_MODULES_TYPE_value}.
| self, | ||
| model: Any, | ||
| smash_config: SmashConfigPrefixWrapper, | ||
| smash_config: SmashConfig | SmashConfigPrefixWrapper, |
There was a problem hiding this comment.
There shouldn't be any change in this file, but there is indeed a type problem here since the file hasn't been updated when changing the return type for the base class' method. I opened a quick PR #540 fixing exactly this
| "coverage", | ||
| "docutils", | ||
| "ty==0.0.1a21", | ||
| "ty==0.0.16", |
There was a problem hiding this comment.
Could we even use 0.0.17 which came out a few days ago please?
|
Thanks for the feedback! I updated the version to I had to add ignore comments in the new signature of |
|
Could you revert all changes that you made concerning this method in this PR, and then re-request review please? |
|
Oh ok, sorry I confused the two versions and kept the old changes when merging the main branch. Bonus: I also bumped to |
6b1eb98 to
b209147
Compare
689075b to
51347cc
Compare
51347cc to
2990010
Compare
gsprochette
left a comment
There was a problem hiding this comment.
LGTM, thanks a lot :)
| if target_modules is None: | ||
| target_modules = self.get_model_dependent_hyperparameter_defaults(model, smash_config)["target_modules"] | ||
| target_modules = cast(TARGET_MODULES_TYPE, target_modules) | ||
| target_modules = self.get_model_dependent_hyperparameter_defaults(model, smash_config) |
There was a problem hiding this comment.
This file should not have changed and needs to be reverted
Description
Updates ty integration and type-checking configuration, and fixes a few type-related issues to keep ty check and existing APIs working cleanly across pruna.
Tooling / Build
0.0.16.uvx ty check src/pruna(avoids checking non-package paths like tests/).possibly-missing-attributeandunused-type-ignore-comment; renamepossibly-unbound-import→possibly-missing-import.Fixes / Compatibility
get_model_dependent_hyperparameter_defaults(...)signatures with the parent method by acceptingSmashConfig | SmashConfigPrefixWrapperacross multiple algorithms.deprecateddetection in SmashConfig by usingSMASH_SPACE.values()rather thanget_hyperparameters().Type-check cleanups
type: ignore[...]where ty flags known-safe patterns (tokenizer token conversion indexing, deprecatedtorch.quantization.quantize_dynamicusage, etc.).Related Issue
Fixes #531
Type of Change
How Has This Been Tested?
pre-commit run -auvx ty check src/prunaChecklist