search

materialsproject / jobflow

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

BUG: append_name for Flows generated as a Response #639

Closed JaGeo closed 2 months ago

JaGeo commented 3 months ago

I am trying to implement a more complex workflow (see https://github.com/materialsproject/atomate2/pull/903). I need to renae jobs to work on the corresponding tests.

from jobflow import job, Flow, Response, run_locally

@job
def my_function(my_parameter: int) -> int:
    return my_parameter

@job
def my_second_function(my_parameter: int) -> int:
    job2 = my_function(my_parameter)
    return Response(replace=Flow(job2))

job3 = my_second_function(2)
job3.append_name(" test")
run_locally(job3)

This results in

2024-07-03 15:40:30,217 INFO Started executing jobs locally
2024-07-03 15:40:30,337 INFO Starting job - my_second_function test (bfde76e4-b636-416b-9fb9-ada2577f7377)
2024-07-03 15:40:30,339 INFO Finished job - my_second_function test (bfde76e4-b636-416b-9fb9-ada2577f7377)
2024-07-03 15:40:30,339 INFO Starting job - my_function (653a9993-4c4e-4332-9321-0f50ce985a55)
2024-07-03 15:40:30,340 INFO Finished job - my_function (653a9993-4c4e-4332-9321-0f50ce985a55)
2024-07-03 15:40:30,340 INFO Finished executing jobs locally

Is there another way to rename that I am overlooking? I would have hoped that "my_function" also gets renamed.

davidwaroquiers commented 3 months ago

Hi @JaGeo

Not sure if it is a bug but at least it is definitely a use case. Somehow what's missing is the dynamic application of the append procedure, similar to e.g. what is done with update_metadata (in which there is a dynamic argument, by default True).

I'll let @utf comment on this but it seems reasonable to add this feature.

utf commented 3 months ago

I agree with @davidwaroquiers, we could add a dynamic option as we did here: https://github.com/materialsproject/jobflow/blob/11034dd00ad0ed64a027560a4f040c5b8c657193/src/jobflow/core/job.py#L935-L937

JaGeo commented 3 months ago

@utf @davidwaroquiers adding a dynamic option would be great and would help with solving my issue.

davidwaroquiers commented 3 months ago

I guess it would be good (and more consistent) to also add the same filtering options. Should then this stay within append_name or be a new update_name method?