Skip to content

Fix type issue in worker.py#512

Merged
hirosassa merged 8 commits into
m3dev:masterfrom
hirosassa:refactor-worker
May 15, 2026
Merged

Fix type issue in worker.py#512
hirosassa merged 8 commits into
m3dev:masterfrom
hirosassa:refactor-worker

Conversation

@hirosassa

Copy link
Copy Markdown
Collaborator

What I did

Resolve all Pyright type errors in gokart/worker.py (23 → 0):

  • Move DISABLED, DONE, FAILED, PENDING, UNKNOWN from luigi.scheduler to luigi.task_status
  • Use dict literals for config_path — Pyright treats dict() calls as dict[str, str], which is not assignable to ConfigPath (TypedDict). Dict literals {...} are correctly inferred as TypedDict-compatible
  • Add type narrowing for flatten() results — Luigi's flatten() returns list[Any], so elements can be str. Added cast(Task, ...) and isinstance guards before accessing Task-specific attributes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR focuses on eliminating Pyright type errors by tightening/clarifying typings around Luigi task status constants, config path typing, and flatten/mapping utilities used across worker/task execution and GCS metadata serialization.

Changes:

  • Updated gokart/worker.py to import task status constants from luigi.task_status, adjusted config_path to TypedDict-friendly literals, and added type narrowing/casts around Luigi flatten()/dependency iteration.
  • Refined FlattenableItems typing in gokart/utils.py (explicit list | tuple | dict recursion) and aligned mapping logic with the narrowed union.
  • Added casts/narrowing in gokart/task.py and gokart/gcs_obj_metadata_client.py to satisfy static typing for nested flattenable structures.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
gokart/worker.py Adjusts Luigi imports and adds type narrowing/casts around dependencies/outputs; updates Config config_path typing.
gokart/utils.py Narrows FlattenableItems TypeAlias and updates map_flattenable_items branching accordingly.
gokart/task.py Adds explicit narrowing/casts for requires() and required_task_outputs construction to satisfy static typing.
gokart/gcs_obj_metadata_client.py Updates flattenable traversal typing for required task outputs serialization and adjusts accepted container types.
Comments suppressed due to low confidence (1)

gokart/gcs_obj_metadata_client.py:136

  • The TypeError message says "dict, or iterable" but this branch only accepts list | tuple. Either broaden the implementation back to collections.abc.Iterable (while still excluding str) or update the error message to match the actually supported types so callers aren’t misled.
            return {k: GCSObjectMetadataClient._get_serialized_string(v) for k, v in required_task_outputs.items()}
        elif isinstance(required_task_outputs, list | tuple):
            return [GCSObjectMetadataClient._get_serialized_string(ro) for ro in required_task_outputs]
        else:
            raise TypeError(
                f'Unsupported type for required_task_outputs: {type(required_task_outputs)}. '
                'It should be RequiredTaskOutput, dict, or iterable of RequiredTaskOutput.'
            )

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread gokart/worker.py Outdated
if not requires.complete(self.check_complete):
# not all requirements are complete, return them which adds them to the tree
new_deps = [(t.task_module, t.task_family, t.to_str_params()) for t in requires.flat_requirements]
new_deps = [(t.task_module, t.task_family, t.to_str_params()) for t in requires.flat_requirements if isinstance(t, Task)]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also agree copilot changes, because this change will drop some tasks unexpectedly

@hirosassa hirosassa May 15, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback! I agree that silently dropping non-Task elements is risky. I've changed this to use cast(list[Task], requires.flat_requirements) instead of the isinstance filter.
This keeps the runtime behavior identical to the original code (a non-Task element would still raise an AttributeError immediately) while satisfying the type checker.

0170810

Comment thread gokart/worker.py Outdated
hirosassa and others added 2 commits May 11, 2026 07:39
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment thread gokart/worker.py Outdated
if not requires.complete(self.check_complete):
# not all requirements are complete, return them which adds them to the tree
new_deps = [(t.task_module, t.task_family, t.to_str_params()) for t in requires.flat_requirements]
new_deps = [(t.task_module, t.task_family, t.to_str_params()) for t in requires.flat_requirements if isinstance(t, Task)]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also agree copilot changes, because this change will drop some tasks unexpectedly

@hirosassa hirosassa enabled auto-merge (squash) May 15, 2026 00:01
@hirosassa hirosassa disabled auto-merge May 15, 2026 00:03

@hiro-o918 hiro-o918 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
thank you for your PRs!

@hirosassa hirosassa merged commit d84c099 into m3dev:master May 15, 2026
8 checks passed
@hirosassa hirosassa deleted the refactor-worker branch May 15, 2026 06:36
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.

3 participants