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

fix: add handling for wrapped run to pickle #401

Closed
wants to merge 3 commits into from
Closed

Conversation

hiro-o918
Copy link
Contributor

@hiro-o918 hiro-o918 commented Oct 13, 2024

Backround

As CI of a8d23f7 shows, when wrapped run function cannot be pickled when the instance parameters to wrap run different
image

gokart.TaskOnKart is pickled when it is passed to workers.
So, the current implementation can cause pickle errors in some conditions, though the parameters of tests in a8d23f7 are tricky.

What

This PR overwrites built-in functions which are used on pickle.dump and pickle.load.
The error mentioned above is fixed by unwrapping/wraping the run function in the pickle process

@hiro-o918 hiro-o918 changed the title feat: add tests to check tasks can be pickled fix: add handling for wrapped run to pickle Oct 13, 2024
@hiro-o918 hiro-o918 marked this pull request as ready for review October 13, 2024 09:16
task_lock.release()
logger.debug(f'Task RUN lock of {task_lock_params.redis_key} released.')
scheduler.shutdown()
return result
Copy link
Contributor

Choose a reason for hiding this comment

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

Though I cannot remember, there must be some reason that this method has return, so I think we should reconsider before removing it.

@hiro-o918 hiro-o918 marked this pull request as draft October 15, 2024 12:20
@hiro-o918 hiro-o918 closed this Oct 28, 2024
@hiro-o918 hiro-o918 deleted the fix/run-wrapper branch October 28, 2024 16:57
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.

2 participants