Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add serializable parameter #411

Merged
merged 1 commit into from
Nov 29, 2024
Merged

Conversation

hiro-o918
Copy link
Contributor

@hiro-o918 hiro-o918 commented Nov 27, 2024

What

  • add an object parameter implementing gokart_serialize and gokart_desirialize function

Why

  • currently we need to list all the parameter to a task
    • luigi.Parameters always flattend on the task
    • luigi.Config also cannot be passed as a config object
      • we need inherit it and use as flatten parameters
  • make a set of parameters are portable to be passed to a task

For example

The following example shows that this feature makes passing a set of parameters among tasks

@dataclass(frozen=True)
class Config:
    foo: int
    bar: str

    def gokart_serialize(self) -> str:
        # dict is ordered in Python 3.7+
        return json.dumps(asdict(self))

    @classmethod
    def gokart_deserialize(cls, s: str) -> 'Config':
        return cls(**json.loads(s))


class FooTask(TaskOnKart):
    config: Config = SerializableParameter(object_type=Config)

    def run(self):
        self.dump(self.config)

class BarTask(TaskOnKart):
    config: Config = SerializableParameter(object_type=Config)

    def run(self):
        self.dump(self.config)

class Pipeline(TaskOnKart):
    config: Config = SerializableParameter(object_type=Config)
    def requires(self):
        return dict(foo=FooTask(config=self.config), bar=barTask(config=self.config),

@hiro-o918 hiro-o918 force-pushed the feature/serializable-parameter branch 4 times, most recently from aa2c617 to 44a6e94 Compare November 27, 2024 13:49
@hiro-o918 hiro-o918 force-pushed the feature/serializable-parameter branch 2 times, most recently from b6eb103 to 28e9559 Compare November 27, 2024 16:09
@hiro-o918 hiro-o918 force-pushed the feature/serializable-parameter branch from 28e9559 to f6453ad Compare November 28, 2024 07:29
Copy link
Member

@kitagry kitagry left a comment

Choose a reason for hiding this comment

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

LGTM

@yokomotod
Copy link
Collaborator

@hiro-o918 lgtm!

Copy link
Collaborator

@yokomotod yokomotod left a comment

Choose a reason for hiding this comment

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

[memo]

I thought we may want more specific Parameter for json encode/decod-able objects, then we don't need to make custom wrapper class.

... -> fine, we can make later if we really need it.

Then, the name of SerializableParameter won't conflict / make confuse ?

... -> seems fine, we can name JsonSerializableParameter or something else.

@hiro-o918
Copy link
Contributor Author

@yokomotod
Nice comment.
As you mentioned,SerializableParameter (this PR) is a generic purpose one.
If we need more specific parameter, we can chose a detailed name.

@hiro-o918 hiro-o918 merged commit 15737f6 into master Nov 29, 2024
5 checks passed
@hiro-o918 hiro-o918 deleted the feature/serializable-parameter branch November 29, 2024 01:07
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