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

ref: missing adds --jobs #3199

Merged
merged 9 commits into from
Jan 28, 2022
Merged

Conversation

daavoo
Copy link
Contributor

@daavoo daavoo commented Jan 20, 2022

  • And update cmd list in jobs config option

@daavoo daavoo added the C: ref Content of /doc/*-reference label Jan 20, 2022
@daavoo daavoo changed the title ref: Add missing --jobs option. ref: add: Describe missing --jobs option. Jan 20, 2022
@shcheklein shcheklein temporarily deployed to dvc-org-ref-add-missing-nhz5ji January 20, 2022 10:36 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-ref-add-missing-nhz5ji January 20, 2022 10:37 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-ref-add-missing-nhz5ji January 20, 2022 11:10 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-ref-add-missing-nhz5ji January 20, 2022 11:16 Inactive
@daavoo daavoo self-assigned this Jan 20, 2022
@daavoo daavoo requested a review from a team January 20, 2022 11:16
@jorgeorpinel jorgeorpinel changed the title ref: add: Describe missing --jobs option. ref: missing adds --jobs Jan 27, 2022
@jorgeorpinel jorgeorpinel added the 🐛 type: bug Something isn't working. label Jan 27, 2022
Comment on lines 172 to 176
- `-j <number>`, `--jobs <number>` - Only used if `--to-remote` is also passed.
Parallelism level for DVC to upload data to remote storage. The default value
is 4 \* cpu_count(). For SSH remotes, the default is 4. Note that the default
value can be set using the jobs config option with dvc remote modify. Using
more jobs may speed up the operation.
Copy link
Contributor

Choose a reason for hiding this comment

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

For closer consistency with other refs that have this option:

Suggested change
- `-j <number>`, `--jobs <number>` - Only used if `--to-remote` is also passed.
Parallelism level for DVC to upload data to remote storage. The default value
is 4 \* cpu_count(). For SSH remotes, the default is 4. Note that the default
value can be set using the jobs config option with dvc remote modify. Using
more jobs may speed up the operation.
- `-j <number>`, `--jobs <number>` -
parallelism level for DVC to transfer data when using `--to-remote`. The default value
is `4 \* cpu_count()`. For SSH remotes, the default is `4`. Using
more jobs may speed up the operation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Committed. I removed "Note that the default value can be set using the jobs config option with dvc remote modify" too. It's a good note though, but it kind of overcomplicates the already-complicated description. I wonder if there's a better place to comment on this... ⌛

Copy link
Contributor

Choose a reason for hiding this comment

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

removed "Note that the default value can be set using the jobs config option...

Maybe we should change the text (in all refs) to something like "The default is 4*cpu (can be set with dvc remote modify jobs) :...

Linked to https://dvc.org/doc/command-reference/remote/modify#available-parameters-for-all-remotes

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-ref-add-missing-nhz5ji January 27, 2022 06:01 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-ref-add-missing-nhz5ji January 27, 2022 06:02 Inactive
jorgeorpinel
jorgeorpinel previously approved these changes Jan 27, 2022
Comment on lines +172 to +174
- `-j <number>`, `--jobs <number>` - parallelism level for DVC to transfer data
when using `--to-remote`. The default value is `4 \* cpu_count()`. For SSH
remotes, the default is `4`. Using more jobs may speed up the operation.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if --jobs also has this same effect in import-url --to-remote. Currently it only refers to downloading the target data. @dberenbaum or @efiop, do you know? Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

--jobs affects the uploading when using dvc import-url --to-remote so the description should be updated. This also affects dvc update

This comment was marked as resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Moved this Q to #3233

@jorgeorpinel jorgeorpinel dismissed their stale review January 27, 2022 06:25

I found another spot missing add mention...

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Jan 27, 2022

@daavoo thanks again for this. Mind also completing the list of commands that have --jobs in https://dvc.org/doc/command-reference/remote/modify#available-parameters-for-all-remotes ?

@daavoo
Copy link
Contributor Author

daavoo commented Jan 27, 2022

@daavoo thanks again for this. Mind also completing the list of commands that have --jobs in https://dvc.org/doc/command-reference/remote/modify#available-parameters-for-all-remotes ?

Done.

@jorgeorpinel jorgeorpinel requested a deployment to dvc-org-ref-add-missing-nhz5ji January 28, 2022 02:20 Abandoned
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-ref-add-missing-nhz5ji January 28, 2022 02:21 Inactive
@jorgeorpinel jorgeorpinel merged commit 7a830dc into master Jan 28, 2022
@jorgeorpinel jorgeorpinel deleted the ref-Add-missing-`--jobs`-option branch January 28, 2022 02:21
iesahin pushed a commit that referenced this pull request Apr 11, 2022
* Update add.md

* Restyled by prettier (#3200)

Co-authored-by: Restyled.io <[email protected]>

* Update add.md

* Restyled by prettier (#3201)

Co-authored-by: Restyled.io <[email protected]>

* Update content/docs/command-reference/add.md

* Restyled by prettier (#3227)

Co-authored-by: Restyled.io <[email protected]>

* Add missing commands with `--jobs`.

* Update content/docs/command-reference/remote/modify.md

* Restyled by prettier (#3234)

Co-authored-by: Restyled.io <[email protected]>

Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com>
Co-authored-by: Restyled.io <[email protected]>
Co-authored-by: Jorge Orpinel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: ref Content of /doc/*-reference 🐛 type: bug Something isn't working.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants