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

validate_resource_spec ignores valid keys that are not MPI-related #3498

Closed
stevenstetzler opened this issue Jun 27, 2024 · 3 comments
Closed

Comments

@stevenstetzler
Copy link

Describe the bug
validate_resource_spec in use by the HighThroughputExecutor declares these legal keys:

    legal_keys = set(("ranks_per_node",
                      "num_nodes",
                      "num_ranks",
                      "launcher_options",
                      ))

for the resource spec. It will raise an InvalidResourceSpecification exception if passed "non-legal" keys even when mpi-mode is disabled. In reality, other keys might be defined in this dictionary, e.g. cores, memory, or disk which is typically used by the WorkQueueExector. Right now, an exception is raised if any keys are included that are not these legal keys, which are all MPI related.

To Reproduce
Submit a task to the HighThroughputExecutor with resource_spec = {'cores': 1}.

Expected behavior
I expected validate_resource_spec to pass if the resource spec contains valid keys, even if those keys will not be used by the executor.

Environment

  • Parsl version: 2024.06.24
@stevenstetzler
Copy link
Author

I'm trying to come up with a PR to address this. It seems to require re-working what is considered an "invalid" key. I'd propose specifying what keys are required to be present under specified conditions e.g. is_mpi_enabled=True and ignoring unneeded keys. Does this sound okay and should I submit something, or would you classify this as user error?

@benclifford
Copy link
Collaborator

ignoring unused keys is an usability problem in itself that I am not fond of - if you specify cores on an executor that doesn't understand that, that executor should be telling you that, not leaving you to struggle with understanding why your code is running weirdly.

The keys for executors are pretty necessarily different for each executor and I think it's good to lead users into understanding those differences rather than pretend there's a uniform interface that isn't and leave them to flail around.

That maybe leads to wanting different mechanisms for specifying resource specifications for the same apps running on different executors, compared to the current way of doing it, although I've done things in the past where a resource specification is generated dynamically based on what you think you want and what the underlying executor can support, using a function...

@benclifford
Copy link
Collaborator

see PR #3519 for the opposite of this issue, and the part of PR #3582 which is to do with validation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants