feat: support type-safe Parameter annotations without mypy plugin#496
feat: support type-safe Parameter annotations without mypy plugin#496
Conversation
| foo = luigi.IntParameter() | ||
| bar = luigi.DateParameter() | ||
| baz = gokart.TaskInstanceParameter() | ||
| qux = luigi.NumericalParameter(var_type=int) | ||
| quux = luigi.ChoiceParameter(choices=[1, 2, 3], var_type=int) | ||
| corge = luigi.EnumParameter(enum=MyEnum) |
There was a problem hiding this comment.
I think we should drop support for non-annotated attributes. Since luigi.Task is decorated with dataclass_transform, it ignores attributes without type annotations by default, so type checking won't work correctly.
| if sys.version_info >= (3, 11): | ||
| from typing import Unpack | ||
| else: | ||
| from typing_extensions import Unpack |
There was a problem hiding this comment.
📝
typing_extensions that a correctly installed.
There was a problem hiding this comment.
Pull request overview
This PR updates gokart’s Luigi parameter typing strategy to use descriptor-based annotations (luigi.XxxParameter / luigi.Parameter[T]) so type checkers can infer the instance value types without requiring the gokart mypy plugin. It also upgrades Luigi to >=3.8.0 to rely on Luigi’s newer descriptor behavior.
Changes:
- Upgrade Luigi dependency to
>=3.8.0and adjust lockfile accordingly. - Migrate internal code, tests, and docs to the new parameter annotation style; make task-parameter wrappers generic for better inference.
- Remove mypy plugin configuration and slim plugin-focused tests.
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Dependency resolution updates (notably Luigi 3.8.0) and platform marker splits. |
| pyproject.toml | Require luigi>=3.8.0 and remove mypy plugin configuration. |
| gokart/task.py | Re-annotate core TaskOnKart parameters using Luigi descriptor typing/generics. |
| gokart/parameter.py | Make TaskInstanceParameter / ListTaskInstanceParameter generic; type SerializableParameter / ZonedDateSecondParameter. |
| gokart/worker.py | Re-annotate gokart_worker config parameters to the new style and add typing ignores where needed. |
| gokart/run.py | Add typing ignores for Luigi retcode mutation. |
| gokart/info.py | Update tree_info parameter annotations to new style. |
| gokart/gcs_config.py | Update GCSConfig parameter annotation to new style. |
| gokart/testing/check_if_run_with_empty_data_frame.py | Update test-run parameter annotations to new style. |
| test/test_mypy.py | Reduce mypy-plugin-centric assertions; assert on exit code + stdout/stderr explicitly. |
| test/test_pandas_type_check_framework.py | Change rerun to a Luigi parameter (instead of a plain boolean). |
| test/test_cache_unique_id.py | Adjust dynamic parameter reassignment to call __set_name__ for Luigi 3.8+ descriptors. |
| test/test_build.py | Update task parameter annotations to new style, including TaskInstanceParameter generics. |
| test/test_run.py | Update task parameter annotation to new style. |
| test/test_worker.py | Update task parameter annotation to new style. |
| test/test_task_on_kart.py | Update parameter annotations + adjust list default to tuple for ListParameter. |
| test/tree/test_task_info.py | Update task parameter annotations and TaskInstanceParameter generics. |
| test/test_task_instance_parameter.py | Update annotations for generic TaskInstanceParameter / ListTaskInstanceParameter and add type ignores for intentional misuse cases. |
| test/test_list_task_instance_parameter.py | Update TaskInstanceParameter generic annotations. |
| test/test_restore_task_by_id.py | Update parameter annotations and add missing Any import for generics. |
| test/test_serializable_parameter.py | Update SerializableParameter to use generics in annotations. |
| test/test_zoned_date_second_parameter.py | Update ZonedDateSecondParameter annotation style. |
| docs/tutorial.rst | Update examples to new parameter annotation style. |
| docs/task_settings.rst | Update example parameter annotations to new style. |
| docs/task_parameters.rst | Update parameter examples to new style (incl. SerializableParameter generics). |
| docs/task_on_kart.rst | Update examples to new parameter annotation style. |
| docs/setting_task_parameters.rst | Update examples to new parameter annotation style and trim trailing whitespace. |
| docs/intro_to_gokart.rst | Update example to new parameter annotation style. |
| README.md | Update TaskInstanceParameter examples to reflect new generic typing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gokart/worker.py
Outdated
| ) | ||
| wait_interval = luigi.FloatParameter(default=1.0, config_path=dict(section='core', name='worker-wait-interval')) | ||
| wait_jitter = luigi.FloatParameter(default=5.0) | ||
| wait_interval: luigi.BoolParameter = luigi.FloatParameter( |
There was a problem hiding this comment.
wait_interval is annotated as luigi.BoolParameter but initialized with luigi.FloatParameter. This breaks type checking and is inconsistent with how the value is used later (numeric comparisons in Worker.__init__). Update the annotation to luigi.FloatParameter (and/or luigi.Parameter[float]) to match the actual parameter type.
| wait_interval: luigi.BoolParameter = luigi.FloatParameter( | |
| wait_interval: luigi.FloatParameter = luigi.FloatParameter( |
gokart/worker.py
Outdated
| id: luigi.Parameter = luigi.Parameter(default='', description='Override the auto-generated worker_id') | ||
| ping_interval: luigi.FloatParameter = luigi.FloatParameter( |
There was a problem hiding this comment.
The new annotation style is intended to be type-safe, but id is annotated as bare luigi.Parameter (which type checkers will treat as Parameter[Any]). Consider using luigi.Parameter[str] (or a more specific parameter class) so self._config.id resolves to str rather than Any.
gokart/parameter.py
Outdated
| import luigi | ||
| from luigi import task_register | ||
| from luigi.parameter import _no_value, _NoValueType, _ParameterKwargs |
There was a problem hiding this comment.
This imports Luigi private/internal names (_no_value, _NoValueType, _ParameterKwargs) from luigi.parameter. Depending on private symbols makes the package brittle across Luigi releases. Prefer avoiding private imports (e.g., use a local sentinel for the default and type **kwargs as Any/Mapping[str, Any], or import these conditionally with a compatibility fallback).
| pandas: bool = luigi.BoolParameter() | ||
| namespace: str | None = luigi.OptionalParameter( | ||
| pandas: luigi.BoolParameter = luigi.BoolParameter() | ||
| namespace: luigi.OptionalParameter = luigi.OptionalParameter( |
There was a problem hiding this comment.
namespace used to be typed as str | None, but the new annotation luigi.OptionalParameter loses the value type (it becomes Any). To keep type-safe access, annotate it as luigi.OptionalParameter[str] (or luigi.Parameter[str | None]) instead of the unparameterized form.
| namespace: luigi.OptionalParameter = luigi.OptionalParameter( | |
| namespace: luigi.OptionalParameter[str] = luigi.OptionalParameter( |
gokart/gcs_config.py
Outdated
|
|
||
| class GCSConfig(luigi.Config): | ||
| gcs_credential_name: str = luigi.Parameter(default='GCS_CREDENTIAL', description='GCS credential environment variable.') | ||
| gcs_credential_name: luigi.Parameter = luigi.Parameter(default='GCS_CREDENTIAL', description='GCS credential environment variable.') |
There was a problem hiding this comment.
gcs_credential_name is now annotated as bare luigi.Parameter, which loses the str value type. Annotate it as luigi.Parameter[str] so os.environ.get(self.gcs_credential_name) remains type-safe and avoids Any propagation.
| gcs_credential_name: luigi.Parameter = luigi.Parameter(default='GCS_CREDENTIAL', description='GCS credential environment variable.') | |
| gcs_credential_name: luigi.Parameter[str] = luigi.Parameter(default='GCS_CREDENTIAL', description='GCS credential environment variable.') |
test/test_run.py
Outdated
| class _DummyTask(gokart.TaskOnKart[Any]): | ||
| task_namespace = __name__ | ||
| param = luigi.Parameter() | ||
| param: luigi.Parameter = luigi.Parameter() |
There was a problem hiding this comment.
param is now annotated as bare luigi.Parameter, which makes self.param type Any and weakens the type-safety this PR aims to provide. Prefer luigi.Parameter[str] (or another appropriately typed parameter class) for a string parameter.
| param: luigi.Parameter = luigi.Parameter() | |
| param: luigi.Parameter[str] = luigi.Parameter() |
gokart/worker.py
Outdated
| task_limit: luigi.IntParameter = luigi.IntParameter( | ||
| default=None, # type: ignore |
There was a problem hiding this comment.
task_limit is declared as luigi.IntParameter but its default is None (currently silenced via # type: ignore). If None is a supported value here, prefer an optional-typed parameter (e.g., luigi.OptionalIntParameter or luigi.Parameter[int | None]) to avoid relying on ignores and to keep the config surface explicit.
| task_limit: luigi.IntParameter = luigi.IntParameter( | |
| default=None, # type: ignore | |
| task_limit: luigi.OptionalIntParameter = luigi.OptionalIntParameter( | |
| default=None, |
gokart/info.py
Outdated
| mode: luigi.Parameter = luigi.Parameter(default='', description='This must be in ["simple", "all"].') | ||
| output_path: luigi.Parameter = luigi.Parameter(default='tree.txt', description='Output file path.') |
There was a problem hiding this comment.
mode/output_path are annotated as bare luigi.Parameter, which erases the value type (Any). Since these are string parameters, annotate as luigi.Parameter[str] so accessing self.mode/self.output_path is type-checked as str.
| mode: luigi.Parameter = luigi.Parameter(default='', description='This must be in ["simple", "all"].') | |
| output_path: luigi.Parameter = luigi.Parameter(default='tree.txt', description='Output file path.') | |
| mode: luigi.Parameter[str] = luigi.Parameter(default='', description='This must be in ["simple", "all"].') | |
| output_path: luigi.Parameter[str] = luigi.Parameter(default='tree.txt', description='Output file path.') |
| param_a: luigi.Parameter = luigi.Parameter() | ||
| param_c: luigi.ListParameter = luigi.ListParameter() | ||
| param_d: luigi.IntParameter = luigi.IntParameter(default=1) |
There was a problem hiding this comment.
This doc example now uses luigi.Parameter/luigi.ListParameter without type arguments, which makes the accessed values Any under the new descriptor-based typing. If the goal is “type-safe annotations without a plugin”, consider using generics here (e.g., luigi.Parameter[str], luigi.ListParameter[tuple[str, ...]]) so readers get correct value types in their type checker.
| param_a: luigi.Parameter = luigi.Parameter() | |
| param_c: luigi.ListParameter = luigi.ListParameter() | |
| param_d: luigi.IntParameter = luigi.IntParameter(default=1) | |
| param_a: luigi.Parameter[str] = luigi.Parameter() | |
| param_c: luigi.ListParameter[tuple[str, ...]] = luigi.ListParameter() | |
| param_d: luigi.IntParameter[int] = luigi.IntParameter(default=1) |
test/test_worker.py
Outdated
| class _DummyTask(gokart.TaskOnKart[str]): | ||
| task_namespace = __name__ | ||
| random_id = luigi.Parameter() | ||
| random_id: luigi.Parameter = luigi.Parameter() |
There was a problem hiding this comment.
random_id is now annotated as bare luigi.Parameter, so the value type becomes Any. Since random_id is used as a string (uuid.uuid4().hex), annotate it as luigi.Parameter[str] to preserve type checking.
| random_id: luigi.Parameter = luigi.Parameter() | |
| random_id: luigi.Parameter[str] = luigi.Parameter() |
Summary
Previously, gokart recommended annotating task parameters like this:
This pattern required the gokart mypy plugin to provide correct type checking, and was incompatible with other type checkers such as Pyright, Pyrefly, and ty.
This PR introduces a new annotation style using
__get__/__set__descriptors built intoluigi.Parameter:With this style, the parameter's generic type (e.g.
luigi.Parameter[int]) correctly resolves to the underlying value type (int) when accessed on an instance, without needing any type checker plugin.Changes
luigi.XxxParameterinstead of bare Python types)TaskInstanceParameterandListTaskInstanceParametergeneric (Generic[T]) for proper typeinference
SerializableParameterandZonedDateSecondParameteruseluigi.Parameter[T]genericsMigration
The old style still works but requires the gokart mypy plugin. The new style works out-of-the-box with
mypy, Pyright, Pyrefly, and ty — no plugin needed.
count: int = luigi.IntParameter()count: luigi.IntParameter = luigi.IntParameter()