New option `jobs` for `dvc import` by karajan1001 · Pull Request #4977 · treeverse/dvc

karajan1001 · 2020-11-26T14:34:43Z

❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
doc for dvc import --jobs dvc.org#2008

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

It works on my computer now, but some more tests are still needed.

`jobs` option for `dvc import`

shcheklein · 2020-11-26T15:34:20Z

+        "--jobs",
+        type=int,
+        help=(
+            "Number of jobs to run simultaneously. "


I know this might come from some other DVC commands, but let's reconsider this message please?

Number of jobs is not very informative. Number of parallel connections? Number of download jobs?

@shcheklein Let's not do that please, it is totally out of scope for this PR.

Though I do see your point 🙁

Could just refer to dvc pull/fetch/status here, to smooth this out. E.g. Please refer to dvc fetch help for description or something like that.

I'm not referring to push/pull or any other commands (we can create a ticket for this if needed). this message in my comment was about this specific help message only.

@shcheklein I understand that. This one was copied over from push/pull/etc, so the questions might arise there as well. If we use Please refer to dvc fetch ... here we'll dodge the bullet 😄 While still keeping the analogy correct, since this is pretty much a pull from an external repo.

hmm ... I guess, my take on this it's fine to change only here (and later propagate if needed, and if it's needed at all). Also Please refer to dvc fetch will force us to kinda go and see fetch, and what's there, etc, etc. Also it complicates UX. To be honest, I haven't see that kind of redirects in the help messages.

I agreed that we can change it here, and submit another one for push/pull/etc.

In fact per #4838 import --jobs option has a special meaning? "This external tracked data might be stored in a remote DVC repository. In this situation --job which controls the parallelism level for DVC to download data from remote storage." So seems like it's not really the same as in other commands?

Please refer to dvc fetch will force us to kinda go and see fetch... I haven't see that kind of redirects in the help messages.

Unrelated, but dvc get-url -h used to refer to import-url (for the url arg. details) I think. Now only import-url has the full list of URLs.

@jorgeorpinel dvc import clones an external repo, and then pull down the data. So I think it means the same as what in dvc pull.

efiop

Looks good! Please check the unit tests, looks like you need to add the new flag there.

FAILED tests/unit/command/test_imp.py::test_import_no_exec - AssertionError: ...
FAILED tests/unit/command/test_imp.py::test_import - AssertionError: Expected...

karajan1001 · 2020-11-29T05:47:55Z

@efiop

There are two problem left

jobs argument is not stored in the dvcfile.

Now we have ways of setting jobs in dvc import and dvc pull after cloning a project with dvc import.
But we can't set it in dvc repro if the imported dataset changed.

help message mentioned above.

FAILED tests/func/test_api.py::test_open_external[azure]
Is this fail caused by this PR?

efiop · 2020-11-29T17:19:57Z

jobs argument is not stored in the dvcfile.

@karajan1001 Great point! For now we could just keep it as is and not include jobs into dvcfile. jobs kinda depends on your particular connection to the server, so it doesn't feel right to have it in dvcfile for everyone. Feels like there should be a config-level thing to set it. There is a ticket for it already, IIRC. So I suggest just leaving as is for now. Very good observation though!

help message mentioned above.

👍

Is this fail caused by this PR?

No, that's just azure being flaky :( Please don't mind it.

efiop · 2020-11-29T17:25:14Z

                Stage.PARAM_ALWAYS_CHANGED,
                Stage.PARAM_MD5,
                Stage.PARAM_DESC,
+                Stage.PARAM_JOBS,


Let's not add it, this is too specific for your particular connection and might be undesired for other users.

Ah, I see that you don't save it, you just utilize the loading this way. Hm, I guess we could keep it as is 👍 Let me see...

Yes, without this, I can't create a stage with the jobs member variable. Stage params' saving is in dvc.schema.py.

efiop · 2020-11-29T17:28:34Z


        if not self.frozen and self.is_import:
-            sync_import(self, dry, force)
+            sync_import(self, dry, force, jobs=self.jobs)


I think you should be able to avoid setting and using self.jobs and PARAM_JOBS. Just pass jobs=... to run in dvc/repo/imp_url.py

I had considered this before but stage.run didn't have any variable before. This represents that it all depends on member variables not on the running status. Adding a variable to it break this, and this would cause some inconsistency of dvc repo.

Feels like there should be a config-level thing to set it. There is a ticket for it already, IIRC. So I suggest just leaving as is for now. Very good observation though!

Maybe we should abandon --jobs and makes config effect?

@karajan1001 Good point! Important consistency here is fetch/pull and import. repro is a bit odd like that with imports, where it tries to run instead of trying to pull. That's something that we are trying to fix with dvc repro --pull flag right now that will become default behaviour in the future.

So ideally there should indeed be a way of telling import that it should load config options from some config section and it should indeed save it in dvcfile. Smth like

remote: config: from-remote: myremote # handy to not have to remember remote name in the project you are importing from

and maybe smth like (pretty rough names, but just talking straight out of my head)

$ dvc import https://.... data --config-from-remote myremote

if we can make a generalization like that, then --jobs would indeed be no longer needed. This would be a more holistic solution, no doubt about that.

efiop · 2020-11-29T17:29:29Z

 def test_import_from_bare_git_repo(
    tmp_dir, make_tmp_dir, erepo_dir, local_cloud
-):
+):  # pylint:disable=unused-argument


I don't think these changes are needed. You probably were missing some dependencies for pylint or something.

It's annoying, I just don't know how to fix this pylint error. I'll try to solve it later.

@karajan1001 It only shows up locally? Try re-installing pip install '.[all,tests]'.

skshetry · 2020-12-04T03:50:11Z

                self.def_repo[self.PARAM_REV_LOCK] = repo.get_rev()

-            _, _, cache_infos = repo.fetch_external([self.def_path])
+            _, _, cache_infos = repo.fetch_external([self.def_path], jobs=jobs)


It wasn't quite apparent how this was being used. Finally, at the 10th/11th level, I found it using repo.cloud.pull(). That's quite deep. :(

Same to me, they need refactoring.

skshetry · 2020-12-04T03:52:39Z


        if not self.frozen and self.is_import:
-            sync_import(self, dry, force)
+            jobs = kwargs.get("jobs", None)


Suggested change

jobs = kwargs.get("jobs", None)

jobs = kwargs.get("jobs")

@skshetry . With this change tests on Windows would fail, I have reverted it.

@karajan1001 Oops, didn't notice this before merging. I think windows tests were failing for an unrelated reason. We've fixed them yesterday, they were failing because of gitpython.

Co-authored-by: Saugat Pachhai <suagatchhetri@outlook.com>

skshetry

Just one suggestion, looks good to me otherwise.

https://github.com/iterative/dvc/pull/4977/files#r535817055

efiop · 2020-12-11T23:32:16Z

Thank you, @karajan1001 ! 🙏

karajan1001 added 2 commits November 26, 2020 21:20

fix treeverse#4838

660ee32

`jobs` option for `dvc import`

option jobs effected

66d1744

shcheklein reviewed Nov 26, 2020

View reviewed changes

efiop reviewed Nov 27, 2020

View reviewed changes

karajan1001 added 2 commits November 29, 2020 09:52

Pass the unit tests parse

53b4316

Adding a functional test.

4c2325c

karajan1001 changed the title ~~[WIP] New option jobs for dvc import~~ New option jobs for dvc import Nov 29, 2020

karajan1001 changed the title ~~New option jobs for dvc import~~ [WIP] New option jobs for dvc import Nov 29, 2020

efiop reviewed Nov 29, 2020

View reviewed changes

Pass jobs to run and remove some pylint

f6fa37b

karajan1001 changed the title ~~[WIP] New option jobs for dvc import~~ New option jobs for dvc import Nov 30, 2020

pmrowla mentioned this pull request Dec 4, 2020

RuntimeError: can't start new thread with dvc import-url #5023

Closed

skshetry reviewed Dec 4, 2020

View reviewed changes

Comment thread dvc/stage/params.py Outdated

Update dvc/stage/__init__.py

ae368c1

Co-authored-by: Saugat Pachhai <suagatchhetri@outlook.com>

skshetry approved these changes Dec 4, 2020

View reviewed changes

remove unused param

5cca616

jorgeorpinel mentioned this pull request Dec 9, 2020

doc for dvc import --jobs treeverse/dvc.org#2008

Merged

Solve test fail on windows

498e94e

efiop approved these changes Dec 11, 2020

View reviewed changes

efiop merged commit 96a97ec into treeverse:master Dec 11, 2020

karajan1001 deleted the fix4838 branch April 7, 2021 07:41

Conversation

karajan1001 commented Nov 26, 2020 • edited by jorgeorpinel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiop Nov 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgeorpinel Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiop left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karajan1001 commented Nov 29, 2020

Uh oh!

efiop commented Nov 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karajan1001 Nov 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiop Nov 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

skshetry left a comment

Choose a reason for hiding this comment

Uh oh!

efiop commented Dec 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

karajan1001 commented Nov 26, 2020 •

edited by jorgeorpinel

Loading

efiop Nov 26, 2020 •

edited

Loading

jorgeorpinel Dec 9, 2020 •

edited

Loading

efiop left a comment •

edited

Loading

efiop commented Nov 29, 2020 •

edited

Loading

karajan1001 Nov 29, 2020 •

edited

Loading

efiop Nov 30, 2020 •

edited

Loading