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

Infer the worker count for tapioca dsl #2089

Merged
merged 2 commits into from
Nov 26, 2024
Merged

Conversation

rzane
Copy link
Contributor

@rzane rzane commented Nov 25, 2024

Motivation

I want tapioca dsl to infer the appropriate number of workers automatically.

Implementation

I noticed the CLI is setting default: 2 for the DSL command, but the other commands don't provide a default. The inference defined here never runs.

I'm assuming this is a mistake, but there could be a good reason for this hardcoded default that I'm not aware of.

Tests

It would be difficult to write a test for this functionality because we'd need to assert on the assumed number of workers in a test for the CLI. The worker count determination happens deep in the stack.

@rzane rzane requested a review from a team as a code owner November 25, 2024 14:07
@KaanOzkan
Copy link
Contributor

This was manually set to 2 after being automatic in 2a5db22. So we'll have to figure out why.

In the meantime it'd be good to have a way to set auto in the tapioca/config.yml so you can still achieve what you want but specific to your project. Feel free to open a PR that does that. Although right now I'm not sure how it'll work because Thor requires a specific type for the option: https://github.com/rails/thor/wiki/method-options. Maybe we could use a value of -1 to indicate auto 😬

@rzane
Copy link
Contributor Author

rzane commented Nov 25, 2024

Yes, I saw that commit, but I couldn't find any context for that decision.

If we find that defaulting to 2 is no longer necessary and merge this PR, is support for --workers -1 still desirable?

@KaanOzkan
Copy link
Contributor

Vini found that it's coming from #618 which may be resolved already so we're discussing if we should merge this PR.

If we find that defaulting to 2 is no longer necessary and merge this PR, is support for --workers -1 still desirable?

Don't think so, no.

@KaanOzkan KaanOzkan added the enhancement New feature or request label Nov 26, 2024
@KaanOzkan KaanOzkan merged commit fc04405 into Shopify:main Nov 26, 2024
15 of 16 checks passed
@rzane rzane deleted the workers branch November 27, 2024 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants