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

add sequential execution mode option #103

Merged
merged 3 commits into from
Jun 5, 2023

Conversation

yu-fuku
Copy link
Contributor

@yu-fuku yu-fuku commented Mar 28, 2023

@mishushakov
Copy link
Member

mishushakov commented Mar 28, 2023

I believe some users would prefer more control over the concurrency (eg. 1 to Infinity)
Also this should be a workflow-level setting, with an option to overwrite from CLI

Have a look at p-map by sindresorhus:
https://github.com/sindresorhus/p-map

Maybe refactor the run function with it accordingly?

@mishushakov
Copy link
Member

mishushakov commented Mar 28, 2023

Btw. sindre has moved all his packages to ESM modules, which we don't support. If you see a related error, just install an older version of p-map

@yu-fuku
Copy link
Contributor Author

yu-fuku commented Mar 29, 2023

Thanks for checking and answering.

I believe some users would prefer more control over the concurrency (eg. 1 to Infinity)

I'd prefer to have user control, as you say.

Let me ask you a few questions

Also this should be a workflow-level setting,

Am I correct in understanding that the file specified in the include syntax is the target?
(I think only one workflow will work unless you specify multiple files in the include)

For example, for the following workflow files, allow the user to choose which of the included files to execute sequentially or in concurrency?

version: "1.1"
name: Status Check
include:
  - sequentially_1.yml
  - sequentially_2.yml
  - concurrency.yml

sequentially_1.yml and sequentially_2.yml are executed sequentially (sequentially_2.yml is executed when sequentially_1yml is completed)
concurrency.yml is parallel execution
Allow users to select files to run sequentially on a file-by-file basis?

with an option to overwrite from CLI

What value do you expect to input from CLI as option?
(Name of files you want to run sequentially(or concurrency)?)
I'm sorry, I can't imagine much about the content of this answer.

@mishushakov
Copy link
Member

Workflow level:

version: "1.1"
name: Status Check
config:
  concurrency: 3 # Run 3 tests concurrently
include:
  - 1.yml
  - 2.yml
  - 3.yml

CLI option:

stepci run workflow.yml --concurrency=3

Hope this helps. Let me know if you need more

@mishushakov
Copy link
Member

When config.concurency is not specified it should default to Infinite

@mishushakov
Copy link
Member

There's also this library, which is similar to p-map:
https://www.npmjs.com/package/p-limit

@yu-fuku
Copy link
Contributor Author

yu-fuku commented Mar 30, 2023

Thanks for replay.
Your answers are so helpful.

You mean to allow parallel execution of included files with specified number of parallel.

Please give me a few days to fix the current implementation and update the pull request.

@yu-fuku yu-fuku force-pushed the sequential_mode branch 2 times, most recently from 45f0ab6 to a354502 Compare March 30, 2023 04:53
@mishushakov
Copy link
Member

mishushakov commented Mar 30, 2023

Of course! Glad to help further.

Yes, if you set concurrency setting is set to 0, the tests should run sequentially, just as you wanted

Don't feel pressured though. In open-source you can contribute at your own pace 😎

@mishushakov
Copy link
Member

mishushakov commented Apr 12, 2023

Hi, @yu-fuku
Let me know if you're stuck and need any help 😁

@yu-fuku
Copy link
Contributor Author

yu-fuku commented Apr 15, 2023

Sorry it took so long to respond.

The number of parallel executions can now be controlled using p-limt.
Please check and comment.

@mishushakov
Copy link
Member

No worries! Glad you took your time to update the pull request.

Have you verified it works?

@yu-fuku
Copy link
Contributor Author

yu-fuku commented Apr 18, 2023

Yes. I have confirmed the following operation.

1, No config.concurrency or cli arguments set → confirm that everything runs in parallel
2, Parallel execution with the number of executions set in config.concurrency
3, Parallel execution with the value set in the cli argument(highest priority)

@mishushakov
Copy link
Member

mishushakov commented Apr 18, 2023

If you set it to 0 does it run in a sequence?

@yu-fuku
Copy link
Contributor Author

yu-fuku commented Apr 18, 2023

Sorry I didn't check when I put the 0 in.

p-limit did not seem to accept values less than 0
https://github.com/sindresorhus/p-limit/blob/v3.1.0/index.js#L5

So I modified runner so that it is executed sequentially when a value less than or equal to 0 is entered.
Please check and comment.

I confirmed that when I incorporate this modification and enter 0, sequential execution is being performed.

Copy link
Member

@mishushakov mishushakov left a comment

Choose a reason for hiding this comment

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

Pretty much there (thanks for updating the pull request). I have very small changes to suggest 😄

src/index.ts Outdated
const testResults = await Promise.all(Object.entries(tests).map(([id, test]) => runTest(id, test, schemaValidator, options, workflow.config, env, credentials)))
let concurrency_num = Object.keys(tests).length

if(workflow.config?.concurrency || workflow.config?.concurrency == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

remove the workflow.config?.concurrency == 0 condition

src/index.ts Outdated
if(workflow.config?.concurrency || workflow.config?.concurrency == 0) {
concurrency_num = workflow.config?.concurrency
}
if(options?.concurrency || options?.concurrency == 0){
Copy link
Member

Choose a reason for hiding this comment

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

remove the options?.concurrency == 0 condition

src/index.ts Outdated
concurrency_num = 1
}

let limit = pLimit(concurrency_num)
Copy link
Member

Choose a reason for hiding this comment

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

let should be const

@mishushakov
Copy link
Member

Hello, let me know if you have any updates?

@fu-yuta
Copy link

fu-yuta commented Jun 5, 2023

sorry I wasn't able to respond to your comment
I have responded now, so could you please check?

@mishushakov
Copy link
Member

No worries, merging it!

@mishushakov mishushakov merged commit 0663956 into stepci:main Jun 5, 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
3 participants