chore(skore): Switch from mypy to ty#2461
Conversation
|
@thomass-dev When you have some time, I would appreciate another review. I’ve updated the PR to address the earlier concerns and verified the CI behavior. |
|
@Sharkyii you probably tagged the wrong person :) |
mypy to ty
mypy to tymypy with ty
Coverage Report for |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| File | Stmts | Miss | Cover | Missing |
|---|---|---|---|---|
| skore-hub-project/src/skore_hub_project | ||||
| __init__.py | 12 | 1 | 91% | 34 |
| exception.py | 2 | 0 | 100% | |
| json.py | 10 | 1 | 90% | 16 |
| protocol.py | 47 | 0 | 100% | |
| skore-hub-project/src/skore_hub_project/artifact | ||||
| __init__.py | 0 | 0 | 100% | |
| artifact.py | 25 | 0 | 100% | |
| serializer.py | 28 | 0 | 100% | |
| upload.py | 36 | 4 | 88% | 175, 177–178, 180 |
| skore-hub-project/src/skore_hub_project/artifact/media | ||||
| __init__.py | 5 | 0 | 100% | |
| data.py | 20 | 0 | 100% | |
| inspection.py | 46 | 1 | 97% | 33 |
| media.py | 10 | 0 | 100% | |
| model.py | 10 | 0 | 100% | |
| performance.py | 45 | 0 | 100% | |
| skore-hub-project/src/skore_hub_project/artifact/pickle | ||||
| __init__.py | 2 | 0 | 100% | |
| pickle.py | 24 | 0 | 100% | |
| skore-hub-project/src/skore_hub_project/authentication | ||||
| __init__.py | 0 | 0 | 100% | |
| apikey.py | 7 | 0 | 100% | |
| login.py | 24 | 3 | 87% | 32–33, 42 |
| token.py | 80 | 0 | 100% | |
| uri.py | 5 | 0 | 100% | |
| skore-hub-project/src/skore_hub_project/client | ||||
| __init__.py | 0 | 0 | 100% | |
| client.py | 63 | 0 | 100% | |
| skore-hub-project/src/skore_hub_project/metric | ||||
| __init__.py | 10 | 0 | 100% | |
| accuracy.py | 35 | 0 | 100% | |
| brier_score.py | 35 | 0 | 100% | |
| log_loss.py | 35 | 0 | 100% | |
| metric.py | 55 | 4 | 92% | 38, 77–78, 84 |
| precision.py | 53 | 0 | 100% | |
| r2.py | 35 | 0 | 100% | |
| recall.py | 55 | 0 | 100% | |
| rmse.py | 35 | 0 | 100% | |
| roc_auc.py | 35 | 0 | 100% | |
| timing.py | 76 | 4 | 94% | 45–46, 104–105 |
| skore-hub-project/src/skore_hub_project/project | ||||
| __init__.py | 0 | 0 | 100% | |
| project.py | 144 | 6 | 95% | 86, 111, 126, 314, 394, 424 |
| skore-hub-project/src/skore_hub_project/report | ||||
| __init__.py | 3 | 0 | 100% | |
| cross_validation_report.py | 73 | 0 | 100% | |
| estimator_report.py | 10 | 0 | 100% | |
| report.py | 59 | 0 | 100% | |
| TOTAL | 1249 | 24 | 98% | |
| Tests | Skipped | Failures | Errors | Time |
|---|---|---|---|---|
| 249 | 0 💤 | 0 ❌ | 0 🔥 | 2m 1s ⏱️ |
Coverage Report for |
|||||||||||||||||||||||||||||||||||||||||||||
| File | Stmts | Miss | Cover | Missing |
|---|---|---|---|---|
| skore-local-project/src/skore_local_project | ||||
| __init__.py | 5 | 0 | 100% | |
| metadata.py | 94 | 5 | 94% | 28, 36, 49, 165–166 |
| project.py | 102 | 1 | 99% | 259 |
| storage.py | 40 | 2 | 95% | 45, 187 |
| TOTAL | 241 | 8 | 96% | |
| Tests | Skipped | Failures | Errors | Time |
|---|---|---|---|---|
| 23 | 0 💤 | 0 ❌ | 0 🔥 | 7.869s ⏱️ |
mypy with tymypy with ty
Why ty flags errors that mypy didn’tImport & attribute resolution
Function call & signature checking
Docs: Assignment & return typing
Docs: Hygiene / redundancy checks
Docs: |
thomass-dev
left a comment
There was a problem hiding this comment.
Just to ensure i can review.
mypy with tymypy to ty
|
|
||
| for warning_class in TRAIN_TEST_SPLIT_WARNINGS: | ||
| warning = warning_class.check(**kwargs) | ||
| warning = cast(Any, warning_class).check(**kwargs) |
There was a problem hiding this comment.
| warning = cast(Any, warning_class).check(**kwargs) | |
| warning = cast(TrainTestSplitWarning, warning_class).check(**kwargs) |
Co-authored-by: Auguste Baum <auguste@probabl.ai>
|
Working on it, hope that's okay ^^' |
|
Yupp, works for me. |
| report = cast(EstimatorReport, report) | ||
| super().__post_init__(report) | ||
|
|
||
| from skore import EstimatorReport |
There was a problem hiding this comment.
This is redundant with the import in TYPE_CHECKING, isn't?
There was a problem hiding this comment.
No, I got test failures when this wasn't imported out of TYPE_CHECKING. So it looks like cast statements are not simply removed at runtime...
|
I'm having a lot of trouble with this because I never get the same check results between CI and my machine. I'm going to let this sit for a while and hope that |
|
|
||
| [tool.mypy] | ||
| exclude = ["hatch/", "tests/"] | ||
| strict = true |
There was a problem hiding this comment.
This change has to be discussed, as ty doesn't have a strict mode for now and we would to be strict as much as possible in skore-hub-project and skore-local-project.
https://docs.astral.sh/ty/reference/typing-faq/#does-ty-have-a-strict-mode
Could you please list me the rules that will not be applied with this change?
| return cast_to_float(getattr(report.metrics, name)(data_source="test")) | ||
|
|
||
| def __post_init__(self, report: EstimatorReport) -> None: # type: ignore[override] | ||
| def __post_init__(self, report: EstimatorReport | CrossValidationReport) -> None: |
There was a problem hiding this comment.
I know it violates the Liskov substitution principle, but your change is not true. Please revert.
There was a problem hiding this comment.
We can fix the violation in a dedicated issue as we have done in https://github.com/probabl-ai/skore/blob/main/skore-hub-project/src/skore_hub_project/report/estimator_report.py, but it is not important here.
| return cast_to_float(series.iloc[0]) | ||
|
|
||
| def __post_init__(self, report: CrossValidationReport) -> None: # type: ignore[override] | ||
| def __post_init__(self, report) -> None: |
Since ty surfaced a large number of pre-existing diagnostics, I added scoped ignore rules in each project’s pyproject.toml to establish a stable baseline:
• skore/: 12 rules ignored
• skore-hub-project/: 5 rules ignored
• skore-local-project/: 5 rules ignored
Fixes: #2376, replaces #2455.