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

TaskVine: Fix serverless functionality #3410

Merged
merged 9 commits into from
Jul 19, 2023

Conversation

tphung3
Copy link
Contributor

@tphung3 tphung3 commented Jul 18, 2023

Changing a task's state to READY inadvertently pushes that task to the list of ready tasks, which is not what we want if that task is a library task. This fixes the issue #3402 and adds test code.
PR #3409 should also be coordinated with this PR for FunctionCall API.

The formatting of results from a FunctionCall is a bit weird, as it is in the json format (see vine_example_function_call.py:49), which implies that results are json-serializable. Maybe cloudpickle is the better alternative here.

@tphung3 tphung3 requested review from dthain and btovar July 18, 2023 20:49
Copy link
Member

@dthain dthain left a comment

Choose a reason for hiding this comment

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

list_rotate was written that way to avoid incurring the cost of a free and then another malloc which are surprisingly expensive in the inner loop of the scheduler.

That said, if it is incorrect or could otherwise be simplified, feel free to propose an improvement.

dthain and others added 4 commits July 19, 2023 08:07
* Improve taskgraph output with minitasks and task names.

* Add name parameter to minitasks.

* Add name parameter to declare_mini_task

* Use mini-task name in node label.

* Update taskgraph example
* Remove labels for large scale graphs.

* Improve graph example

* Fix conda package name
…g-lab#3349)

* create poncho package from dict or string

* restructure

* fix

* sort spec and cache

* fix typo

* add logging info

* fix sort

* add doc

* fix typos

* fix doc + add example
@tphung3
Copy link
Contributor Author

tphung3 commented Jul 19, 2023

PR is updated to conform to PR #3409.

The issue of result formatting (where the returned value is a string in json format) is fine as long as:

  • Users are aware that they have to do json.loads(t.output)['Result'] to get the result. -> More responsibility on users.
  • Results must be json-serializable. For those that are not json-serializable, users must use an implicit result serialization into bytestreams within the submitted functions (e.g., calling dill.dumps(result) right before returning from function execution) as bytestreams are json-serializable. -> More responsibility on users.

@dthain
Copy link
Member

dthain commented Jul 19, 2023

Thanks for disentangling this.

While I am content to let the function results be json-encoded for now, I'm not thrilled with the idea that the user is responsible for decoding from json. It seems to me that taskvine should do it for the user. (And that frees us up to do some other encoding later.)

What do you think?

@tphung3
Copy link
Contributor Author

tphung3 commented Jul 19, 2023

This PR is RTM.

@dthain dthain merged commit 69199e3 into cooperative-computing-lab:master Jul 19, 2023
4 checks passed
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.

4 participants