Remove defaults for Manager init, forcing keywords to be specified, and perhaps revealing a bug that start_method is unused #2973
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nd perhaps revealign a bug that start_method is unused
|
@WardLT tagging you here because start method code came from you... |
benclifford
changed the title
rjmello
requested changes
Dec 12, 2023
rjmello
approved these changes
Dec 12, 2023
benclifford
added a commit
that referenced
this pull request
Mar 29, 2024
This function has a lot of parameters, and long sequences of positional parameters are hard to align. See #2973 for similar work elsewhere.
benclifford
added a commit
that referenced
this pull request
May 28, 2024
there are default values in the interchange code, but they are all specified in the executor code too, so these defautls will never be used. remove them as misleading. see similar changes to process worker pool, PR #2973, for more detailed justification needs to change zmq sockets test because that assumes the arbitrary defaults are present. which is no longer the case. but if you want to initialize an interchange that requires you to specify all this stuff, and want some arbitrary values, then make those arbitrary values yourself. client address parameter is now supplied by the executor - it was not before, and so the default/hard-coded value now lives in the executor, not the interchange
benclifford
added a commit
that referenced
this pull request
Jun 3, 2024
This PR makes all parameters to the Interchange class into mandatory keyword-only arguments. The removed defaults were not used in production use, because they were all specified explicitly in parsl/executors/high_throughput/executor.py too. The single exception to this was client_address, which was defaulted in the interchange and never specified by the exeuctor. This PR moves that default into executor.py too, to work like all the other defaults. See similar changes to the process worker pool, PR #2973, for more detailed justification. test_zmq_binding.py is the only test which instantiates Interchange objects directly (rather than testing the executor as a whole) and this PR modifies that test to explicitly specify all interchange parameters rather than relying on the otherwise-unused defaults.
benclifford
added a commit
that referenced
this pull request
Jun 10, 2024
This PR makes all parameters to the Interchange class into mandatory keyword-only arguments. The removed defaults were not used in production use, because they were all specified explicitly in parsl/executors/high_throughput/executor.py too. The single exception to this was client_address, which was defaulted in the interchange and never specified by the exeuctor. This PR moves that default into executor.py too, to work like all the other defaults. See similar changes to the process worker pool, PR #2973, for more detailed justification. test_zmq_binding.py is the only test which instantiates Interchange objects directly (rather than testing the executor as a whole) and this PR modifies that test to explicitly specify all interchange parameters rather than relying on the otherwise-unused defaults.
benclifford
added a commit
that referenced
this pull request
Jul 18, 2024
See similar changes to the process worker pool, PR #2973, for more detailed justification.
benclifford
added a commit
that referenced
this pull request
Jul 26, 2024
See similar changes to the process worker pool, PR #2973, for more detailed justification.
benclifford
added a commit
that referenced
this pull request
Jul 31, 2024
See similar changes to the process worker pool, PR #2973, for more detailed justification.
benclifford
added a commit
that referenced
this pull request
Aug 15, 2024
See similar changes to the process worker pool, PR #2973, for more detailed justification.
benclifford
added a commit
that referenced
this pull request
Aug 15, 2024
See PR #2973 for justification of mandatory keyword args.
benclifford
added a commit
that referenced
this pull request
Aug 15, 2024
See PR #2973 for justification of mandatory keyword args.
benclifford
added a commit
that referenced
this pull request
Aug 16, 2024
See PR #2973 for justification of mandatory keyword args.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR removes theoretically-always-unused argument defaults in the process worker pool manager, and forces arguments to be supplied as kwargs.
These defaults have two downsides:
i) users reading the code might believe the defaults will be used as end-Parsl-user defaults. However, generally there will be up to two other layers of defaults that will override the defaults removed by this PR: defaults specified in the argparser of process_worker_pool; and defaults specified in the constructor to the HighThroughputExecutor object.
These defaults, then, are misleading, as they are defaults only in the presence of a coding error such as ii) below.
ii) in the presence of a coding error adding a new parameter, where the new parameter is not wired all the way through, rather than raise an error about a missing parameter, the otherwise-unused default is used. This was a practical problem demonstrated by the threads/spawn/fork start method parameter introduced in #2433 and removed as broken in #2980.
Switching to mandatory kwargs can help with accidentally misarranged parameters in the construction of a Manager object, a similar but different coding error to ii) above.
Changed Behaviour
none exposed to user. a class of programming error should be eliminated.
Type of change