materialsproject / jobflow

jobflow is a library for writing computational workflows.
https://materialsproject.github.io/jobflow
Other
90 stars 25 forks source link

FIX: Properly jsanitize fireworks Task #544

Closed gpetretto closed 4 months ago

gpetretto commented 4 months ago

I have seen that in some cases the JobFiretask does not properly serialize a Job. Consider the case of a Maker with a Callable as an attribute.

from fireworks import LaunchPad
from jobflow.managers.fireworks import job_to_firework

from dataclasses import dataclass
from typing import Callable
from jobflow import Maker, job

def print_function():
    print("TEST")

@dataclass
class CallableMaker(Maker):
    f: Callable
    name: str = "CallableMaker"

    @job
    def make(self, *args, **kwargs):
        self.f()

j = CallableMaker(f=print_function).make()

fw = job_to_firework(j)
lpad = LaunchPad.auto_load()
lpad.add_wf(fw)

When running this, in the firework DB you have

{
    "spec": {
        "_tasks": [
            {
                "job": {
                    "@module": "jobflow.core.job",
                    "@class": "Job",
                    "@version": "0.1.14.post122+g6439853",
                    "function": {
                        "@module": "__main__",
                        "@callable": "CallableMaker.make",
                        "@bound": {
                            "@module": "__main__",
                            "@class": "CallableMaker",
                            "@version": null,
                            "f": "<function print_function at 0x16ad82cb0>",
                            "name": "CallableMaker"
                        }
                    },
                   ...

}

Where the function is serialized as a string.

This PR addresses this issue by overriding the JobFiretask.to_dict method and explicitly jsanitizing the Job.

utf commented 4 months ago

Great catch! Thanks @gpetretto

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (c211030) 99.42% compared to head (0debb38) 99.42%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #544 +/- ## ======================================= Coverage 99.42% 99.42% ======================================= Files 21 21 Lines 1556 1564 +8 Branches 422 425 +3 ======================================= + Hits 1547 1555 +8 Misses 9 9 ``` | [Files](https://app.codecov.io/gh/materialsproject/jobflow/pull/544?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject) | Coverage Δ | | |---|---|---| | [src/jobflow/managers/fireworks.py](https://app.codecov.io/gh/materialsproject/jobflow/pull/544?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject#diff-c3JjL2pvYmZsb3cvbWFuYWdlcnMvZmlyZXdvcmtzLnB5) | `100.00% <100.00%> (ø)` | |
utf commented 4 months ago

Is it possible to add a quick test?

gpetretto commented 4 months ago

Is it possible to add a quick test?

done

utf commented 4 months ago

Perfect, thanks so much.