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

feat(cli): Add Concurrency for the make cog cli. #2640

Merged
merged 8 commits into from
Jan 16, 2023

Conversation

Wentao-Kuang
Copy link
Contributor

@Wentao-Kuang Wentao-Kuang commented Jan 12, 2023

Changes

  • Support concurrency in cog creation cli.
  • Make the chunkJobUnit = concurrency*maxChunkUnit to chunk the more jobs with multi concurrency.
  • Fix create-cog to support local source files.

@Wentao-Kuang Wentao-Kuang changed the title feat(cli): Add Concurrency for the create cog cli. feat(cli): Add Concurrency for the make cog cli. Jan 12, 2023
@Wentao-Kuang Wentao-Kuang marked this pull request as ready for review January 12, 2023 03:16
@Wentao-Kuang Wentao-Kuang requested a review from a team as a code owner January 12, 2023 03:16
@@ -113,11 +118,17 @@ export class CommandCogCreate extends CommandLineAction {

const tmpFolder = await makeTempFolder(`basemaps-${job.id}-${CliId}`);

const Q = pLimit(this.concurrency?.value ? this.concurrency.value : DefaultConcurrency);
Copy link
Member

Choose a reason for hiding this comment

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

Use ??

packages/cli/src/gdal/gdal.docker.ts Show resolved Hide resolved
parameterLongName: '--concurrency',
description: 'Number of concurrency create cog jobs',
required: false,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to set the default concurrency value here via defaultValue, rather than in a constant on line 24? That would mean we wouldn't need to check if it had been supplied every time it is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good point. This default value won't be set into this.concurrency.value if this parameter not used, the use case is this.concurrency.defaultValue, so we still need to check exists and set this.concurrency.defaultValue in not exists.

Copy link
Member

Choose a reason for hiding this comment

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

This default value won't be set into this.concurrency.value if this parameter not used, the use case is this.concurrency.defaultValue

That is just a typing problem, and one of the reasons I think we should change our cli parser.

if no value is provided, the this.concurrency.value will be defaultValue

Array.from(names).map((name) =>
Q(async () => {
const tiffJob = await CogStacJob.load(jobLocation);
this.processTiff(tiffJob, name, tmpFolder, isCommit, logger.child({ tiffName: name }));
Copy link
Member

Choose a reason for hiding this comment

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

this should be awaited?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, just fixed.

@kodiakhq kodiakhq bot merged commit d95537f into master Jan 16, 2023
@kodiakhq kodiakhq bot deleted the feat/multi-thread-cog branch January 16, 2023 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants