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

Prepare Nextflow for the CWS Plugin #3727

Merged
merged 7 commits into from
Mar 13, 2023
Merged

Conversation

Lehmann-Fabian
Copy link
Contributor

This PR is necessary to publish my CWS Plugin finally. The plugin is finished, and you find it here.
In the K8sResponseException, I introduce a new field to store the error code.
For GroupKey, I need to get the target to submit all input data to the scheduler.
Besides that, all changes are only about access modifiers to allow inheriting classes access to required fields.

None of the changes influences the current Nextflow behavior or features. Nor are there any side effects.

@bentsherman bentsherman requested a review from pditommaso March 7, 2023 17:16
@pditommaso
Copy link
Member

These are all small changes. However, the side effect of them, internal APIs will be made accessible to components that are not meant to consume them (especially plugins).

What's the value for the scheduling sub-system to access rateLimiter or K8s client error code (and not other executor errors)? As a side thought, maybe that information should be accessed with a dedicated API extension

@Lehmann-Fabian
Copy link
Contributor Author

Thanks for your feedback. Let me explain why I think the following changes are the right way to go.
rateLimiter:
https://github.com/CommonWorkflowScheduler/nf-cws/blob/master/plugins/nf-cws/src/main/nextflow/cws/processor/CWSTaskPollingMonitor.groovy#L50-L77
Here, I overwrite the submitPendingTask method to implement a batching strategy, not to start scheduling before all ready-to-run tasks are submitted. Therefore, I copied the original code and only added three lines for batching. In the original code, you use the submitRateLimiter, which I also want to support. However, I cannot access it because it is private. For sure, I can create a second submitRateLimiter, as the submitRateLimit() method is protected.
https://github.com/nextflow-io/nextflow/blob/master/modules/nextflow/src/main/groovy/nextflow/processor/TaskPollingMonitor.groovy#L285
But using the same instance as the superclass for two reasons would be better.
First, the submitRateLimiter is currently only used in the submitPendingTask method, which I overwrite.
And second, if the TaskPollingMonitor is later adjusted, it might cause problems.

ErrorCode:
It is in general useful to have the errorCode in the Kubernetes Exception.
Nextflow could handle 404 (not found) problems differently than other Exceptions. I often saw whole pipelines failing if Nextflow couldn't delete a pod after a task was successfully finished because of a 404 error. But this is another topic.
However, I deal with 404 differently than with other errors in my plugin.
https://github.com/CommonWorkflowScheduler/nf-cws/blob/master/plugins/nf-cws/src/main/nextflow/cws/k8s/K8SSchedulerClient.groovy#L66
But if you don't allow me to save the error code into the exception, I can parse it from the Exception message. However, this could cause errors if the message is changed in the future.
https://github.com/nextflow-io/nextflow/blob/master/modules/nextflow/src/main/groovy/nextflow/k8s/client/K8sClient.groovy#L675

So it is up to you. If you decline the changes, I will work around it to get my plugin working anyway, but having the changes will prevent me from using redundant values or reflections and String parsing.

@pditommaso pditommaso force-pushed the master branch 2 times, most recently from 0d59b4c to b93634e Compare March 11, 2023 11:20
@pditommaso
Copy link
Member

Here, I overwrite the submitPendingTask method to implement a batching strategy

I see, but using this approach, CWS is strictly coupled with the nextflow implementation. I believe the CWS scheduler should manage the batching independently on the workflow engine. Don't you agree?

The same for the K8s error code

@Lehmann-Fabian
Copy link
Contributor Author

I got your point, and I have an idea of how to avoid copying the code of the TaskPollingMonitor.

The error code is not used for anything scheduling-specific. But as Nextflow is built around the paradigm "Do not install anything," I start the scheduler if necessary at runtime, and there I want to catch more details about the problems. Here, I don't see that I couple to close on Nextflow, but this is definitely an unimportant aspect of my CWS Plugin.

I removed both changes from this PR so that you can merge it, and I can publish the Plugin soon.

Signed-off-by: Lehmann_Fabian <[email protected]>
@Lehmann-Fabian
Copy link
Contributor Author

I have further reduced the PR. Now there are only three unavoidable changes.

Copy link
Member

@pditommaso pditommaso left a comment

Choose a reason for hiding this comment

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

It seems reasonable.

@pditommaso pditommaso merged commit fd8bf70 into nextflow-io:master Mar 13, 2023
@Lehmann-Fabian
Copy link
Contributor Author

Thanks for merging. I'll publish the plugin later today.

@pditommaso
Copy link
Member

You are welcome

abhi18av pushed a commit to abhi18av/nextflow that referenced this pull request Oct 28, 2023
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