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

Refactor 4 different renderings of task description #3743

Merged
merged 9 commits into from
Jan 14, 2025

Conversation

benclifford
Copy link
Collaborator

Prior to this PR, there were 4 variant ways to render a future (which is usually but not always a Parsl task) as a text string:

  1. describing failure of inner future in a join_app with 1 future to join
  • rendered task ID if the inner future has a task record attribute, without checking it is a parsl.dataflow.taskrecord.TaskRecord or that that attribute contains a task id entry.
  • otherwise render as None
  1. describing failure of inner future in a join_app with a list of futures to join
  • is meant to do the same as case 1, but is buggy and always renders as None (the wrong object is checked for having a task_record attribute)
  1. describing failure of a dependency future
  • rendered task ID if there was a task_record, and the
    future is from the same DFK as is rendering the code
    (so that a task number from a different DFK is not
    rendered, in the corner case of a task from one DFK
    being used as a dependency for a task in another DFK)

  • otherwise, renders the repr of the future

  1. describing which dependencies will be waited on when submitting a task to the DFK
  • rendered task ID if the future is an instance of AppFuture or DataFuture
  • otherwise renders the repr of the future

This PR makes a single render method. It is a member of the DFK, because rendering is done in the context of the DFK: two parsl task AppFutures will be rendered differently if they are from this DFK or a different DFK.

This PR implements render_future_description which tries to combine all of the above:

  • if the future is an AppFuture, and is from the same
    DFK, render the task ID
  • otherwise render the repr of the future

Changed Behaviour

human readable descriptions of dependencies/join inner futures will be changed

Fixes

Fixes # (issue)

Type of change

Choose which options apply, and delete the ones which do not apply.

  • Bug fix
  • New feature
  • Update to human readable text: Documentation/error messages/comments
  • Code maintenance/cleanup

+        # TODO: (note in PR): in some cases of refactoring, look for task_record
+        # in other cases look for AppFuture, DataFuture classes...
+        # a type driven one would be better...
+        # with possible slight behaviour changes?

For the four use cases, look at before and after behaviour.
including bugfix
                        # TODO: looks like there was maybe a bug here in the
                        # original code: was looking at joinable, not at
                        # future, so always going down the render-as-future path?
                        # should be easy to test...
@benclifford benclifford marked this pull request as ready for review January 13, 2025 18:03
Copy link
Collaborator

@khk-globus khk-globus left a comment

Choose a reason for hiding this comment

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

Nice improvement.

parsl/tests/test_python_apps/test_fail.py Show resolved Hide resolved
parsl/tests/test_python_apps/test_join.py Show resolved Hide resolved
parsl/tests/test_python_apps/test_join.py Show resolved Hide resolved
@benclifford benclifford added this pull request to the merge queue Jan 14, 2025
Merged via the queue into master with commit 784256d Jan 14, 2025
8 checks passed
@benclifford benclifford deleted the benc-render-future-deps branch January 14, 2025 13:35
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