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

Clarify return value of executor.shutdown #2941

Merged
merged 2 commits into from
Nov 6, 2023

Conversation

benclifford
Copy link
Collaborator

Some executors returned constant True; some returned None. No use is made of this return value, so set it to None in the base definition.

This will break any outside-of-parsl code which actually relies on a None or True being returned.

Type of change

  • Code maintentance/cleanup

Some executors returned constant True; some returned None.
No use is made of this return value, so set it to None in the
base definition.
@benclifford benclifford requested a review from yadudoc November 1, 2023 18:41
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.

Meaningless return value. The function either excepted, or it returned True. I haven't delved completely in, but I don't think we're extending any interface that wants this the RV to be a boolean, so I think this change "should be good."

I do note that parsl.executors.base.ParslExecutor.__exit__ returns False:

# parsl/executors/base.py
...
    def __enter__(self) -> Self:
        return self

    def __exit__(self, exc_type: Any, exc_val: Any, exc_tb: Any) -> Literal[False]:
        self.shutdown()
        return False

... which seems to me of the same order or mindset as this PR.

@benclifford
Copy link
Collaborator Author

I don't think we're extending any interface

originally these were meant to look like executors from the concurrent module - as time has passed they've moved away from that interface enough that it's probably better to refer to them as "inspired by" now.

@benclifford
Copy link
Collaborator Author

I think that False in __exit__ has a purpose:

If an exception is supplied, and the method wishes to suppress the exception (i.e., prevent it from being propagated), it should return a true value. Otherwise, the exception will be processed normally upon exit from this method.

https://docs.python.org/3.9/reference/datamodel.html?highlight=__exit__#object.__exit__

@khk-globus
Copy link
Collaborator

I think that False in __exit__ has a purpose:

Right; but it's summarily returned. As I read that then, all exceptions are ignored when in that context manager, no? As in, the return True was ignored from the Executor, and I was thinking that given the co-location to self.shutdown(), the exceptions being summarily ignored might be related in thought-process (or thought-less-process, if you will).

Perhaps a different conversation than I was thinking, and not related to this PR, but I noted it while reading the codes.

@khk-globus
Copy link
Collaborator

Aaaaand ... I see it now. Too late and I should go to bed. NVM.

@benclifford benclifford merged commit e7b8cd2 into master Nov 6, 2023
4 checks passed
@benclifford benclifford deleted the benc-mypy-executor-shutdown-type branch November 6, 2023 13:21
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