Closed benclifford closed 3 weeks ago
might be interesting to consider and document the difference in meaning vs MissingResourceSpecification, also in mpi_prefix_composer
(and maybe merge?)
/There are three places where the executors raise the same or similar error.
when passing the resource_specification parameter to the htex, it raises the InvalidResourceSpecification from validate_resource_spec method. https://github.com/Parsl/parsl/blob/master/parsl/executors/high_throughput/executor.py#L342
ThreadPoolExecutor also raises the UnsupportedFeatureError when passing resource specification to it, but the raised error is in parsl.executors.error
. https://github.com/Parsl/parsl/blob/master/parsl/executors/threads.py#L54
WorkQueueExecutor raises two errors similar to this but as a Base ExecutorError, one when there is invalid resource_specification ; https://github.com/Parsl/parsl/blob/master/parsl/executors/workqueue/executor.py#L418 and another one when the autolabel is false and resource_specification is not right, https://github.com/Parsl/parsl/blob/master/parsl/executors/workqueue/executor.py#L429
From the above,
1. As ThreadPoolExecutor raises the UnsupportedFeatureError from the parsl.executors.error
, ideally htex should also raise the same instead of raising InvalidResourceSpecification.
What we can do,
parsl.executors.error
. As MissingResourceSpecification is raised only when an empty resource_specification is supplied. 2. htex and threads can raise the UnsupportedFeatureError as that is more reasonable than ResourceSpecificationError.
Is your feature request related to a problem? Please describe. HTEX's mpi_prefix_composer defines an InvalidResourceSpecification. This is used by the non-MPI parts of htex too. WorkQueue raises an ExecutorError for the same kind of error. I have not investigated the other executors.
Describe the solution you'd like
This should be rationalised: probably InvalidResourceSpecification should become an error in
parsl.executors.error
and all executors that validate resource specifications should use it.Additional context
This came from review of PR #3582