Skip to content

Conversation

@hategan
Copy link
Collaborator

@hategan hategan commented Mar 11, 2025

When passing a plain JobExecutorConfig to a batch scheduler executor, the executor attempts to access various batch scheduler specific properties, which do not exist (see #511).

This PR checks if a vanilla JobExecutorConfig is passed to a batch scheduler executor and replaces it with a BatchSchedulerExecutorConfig while copying the relevant properties.

Fixes #511.

Copy link
Collaborator

@andre-merzky andre-merzky left a comment

Choose a reason for hiding this comment

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

This looks correct and complete to me - thanks for the fast fix!

One thing to keep in mind is that _from_config needs updating whenever the JobConfig class changes. Probably not worth to get clever at this point, we can revisit once we stumble over this again (if ever).

@hategan
Copy link
Collaborator Author

hategan commented Mar 11, 2025

One thing to keep in mind is that _from_config needs updating whenever the JobConfig class changes. Probably not worth to get clever at this point, we can revisit once we stumble over this again (if ever).

Indeed. I went with a simple and static solution. We could do it by iterating over properties, but I'm not sure the gains are worth the loss in clarity.

I'm going to wax philosophical, but does any static language have a mechanism to deal with something like this?

@andre-merzky
Copy link
Collaborator

andre-merzky commented Mar 11, 2025

I'm going to wax philosophical, but does any static language have a mechanism to deal with something like this?

In our (very biased) RCT approach we would render all description or config like classes as dictionary like things (we have our own TypedDict base class for that) which then makes copies etc. transparent.

I went with a simple and static solution.

Absolutely the right thing to do - just in case my comments didn't communicate that clearly :-D

@hategan hategan merged commit d49a2a4 into main Mar 11, 2025
12 of 14 checks passed
@hategan hategan deleted the batch_scheduler_settings branch March 11, 2025 21:19
@tesujimath
Copy link

Great, thank you! And thanks for the quick fix. Looking forward to the next release at which time I will switch back to vanilla JobExecutorConfig. 😁

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.

Setting work_directory in abstract class JobExecutorConfig breaks SlurmExecutor with no attribute 'initial_queue_polling_delay'

4 participants