Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
| sequence=[4, 8], | ||
| default_value=8, | ||
| meta=dict(desc="Number of bits to use for quantization."), | ||
| meta=cast(Mapping[Hashable, Any], dict(desc="Number of bits to use for quantization.")), |
There was a problem hiding this comment.
Missing cast() on Boolean meta arguments
Medium Severity
Two Boolean hyperparameter calls still use bare meta=dict(...) without cast(Mapping[Hashable, Any], ...), even though the invalid-argument-type rule was just enabled in pyproject.toml. The sibling file huggingface_llm_int8.py has the exact same Boolean("double_quant", ...) and Boolean("enable_fp32_cpu_offload", ...) calls properly wrapped with cast() (lines 78–85), but these two in huggingface_diffusers_int8.py were missed. This inconsistency will likely cause ty type-check failures now that the rule is enforced.
| def split_train_into_train_val_test(dataset: Dataset, seed: int) -> Tuple[Dataset, Dataset, Dataset]: | ||
| def split_train_into_train_val_test( | ||
| dataset: Dataset | IterableDataset, seed: int | ||
| ) -> Tuple[Dataset, Dataset, Dataset]: |
There was a problem hiding this comment.
Split functions accept IterableDataset but call unsupported method
Medium Severity
The split_train_into_train_val_test, split_train_into_train_val, and split_val_into_val_test functions now accept Dataset | IterableDataset, but their bodies call dataset.train_test_split() which only exists on Dataset, not IterableDataset. Passing an actual IterableDataset would raise an AttributeError at runtime. The callers use cast(Dataset | IterableDataset, ...) which masks the issue, but the widened signatures are misleading and invite future misuse.
Additional Locations (2)
gsprochette
left a comment
gsprochette
left a comment
There was a problem hiding this comment.
Looks good to me, but this PR needs to wait for #535 to be merged and then re-run ty on version 0.0.18
| """ | ||
| ds = load_dataset("argmaxinc/librispeech-200", split="train", streaming=False) | ||
| ds = ds.map(lambda batch: {"sentence": ""}) | ||
| ds = cast(Dataset | IterableDataset, ds) |
There was a problem hiding this comment.
why is this needed? I ran ty check with and without this casting and got the same result
| from pruna.data.pruna_datamodule import PrunaDataModule | ||
| from pruna.evaluation.metrics.aesthetic_laion import AestheticLAION | ||
|
|
||
| _ClipModel = Literal[ |
There was a problem hiding this comment.
Can be removed because this is adding complexity in tests which we don't type check
begumcig
force-pushed
the
fix/ty-invalid-argument
branch
4 times, most recently
from
db237da to
bdd01a3
Compare
begumcig
force-pushed
the
fix/ty-invalid-argument
branch
from
bdd01a3 to
2ff1145
Compare
2 participants
Description
Related Issue
Fixes #(issue number)
Type of Change
How Has This Been Tested?
Checklist
Additional Notes