remote: config checksum_jobs #3133

gurobokum · 2020-01-14T02:43:26Z

Additional details

I would like to suggest to define and use such terms for avoiding ambiguity

config - global repo config
settings - dict of config section
Hence refactor RemoteBase from

    def __init__(self, repo, config):  ->  def __init__(self, repo, settings):

with all changes as config -> settings

Then get some setting as:

Check first into component settings
Check in config if requires
Return default

❗ Have you followed the guidelines in the Contributing to DVC list?
📖 Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.
❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addressed. Please review them carefully and fix those that actually improve code or fix bugs.

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

ghost · 2020-01-14T04:41:21Z

tests/func/test_remote.py

@JIoJIaJIu , sorry for the confusion and extra work, but we are in the middle of a migration from unittest to pytest.
Could you rewrite your tests to pytest? Happy to help with further questions about this)

Could you rewrite your tests to pytest?

Have I got right your request?
gurobokum@42c34d7

@JIoJIaJIu, you did) Thanks!

ghost · 2020-01-14T04:47:49Z

dvc/remote/base.py

Are we going to have both, core and remote checksum_jobs? Why not going remote-only?

I've implemented fallback logic as was described in the comment

@JIoJIaJIu Got it, thanks! Actually, thinking about it, if checksum_jobs didn't work anyway, we could consider just getting rid of it, as it doesn't make much sense in many cases. E.g. if core.checksum_jobs is set for 200, then it makes sense for local remote or s3, but will break everything for ssh remote as we will run out of connections. But that will break the backward compatibility, so not worth bothering with right now. So your current solution is correct, let's stick with it for now 👍

@efiop , so are we staying with both core and remote for backward compatibility issues?

@MrOutis Yep, at least for now.

dvc/remote/base.py

ghost · 2020-01-14T05:02:12Z

Thanks for the PR, @JIoJIaJIu, left some comments :)

Is your suggestion about renaming dvc.config.config to dvc.config.settings?

gurobokum · 2020-01-14T07:34:49Z

Thanks for the PR, @JIoJIaJIu, left some comments :)

Is your suggestion about renaming dvc.config.config to dvc.config.settings?

Hello @MrOutis
No, I meant the part when some component is initialized with partial of the dvc.config
Here for example it has names as a settings -

dvc/dvc/remote/__init__.py

Line 42 in fe635a5

return _get(settings)(repo, settings)

But further in RemoteBASE it has naming as config that introduces ambiguity -

dvc/dvc/remote/base.py

Line 85 in fe635a5

def __init__(self, repo, config):

I suggest just to define such terms as

# whole config
...
[core]  # specific section - core
# everything under section are settings
checksum_jobs = 100

['remote "aws"'] # specific section - remote aws
# everything under section are settings
url = s3://bucket/name 
...

And change

dvc/dvc/remote/base.py

Line 85 in fe635a5

def __init__(self, repo, config):

to

def __init__(self, repo, settings):

So it means that component (Remote) can be initialized with settings which have priority1 and use config if needs as fallback rather then use 2 configs where the one is a child of the second
What do you think?

efiop · 2020-01-14T09:02:48Z

dvc/remote/base.py

No need to pass repo, you have self.repo set already.

The reason why I decided to pass repo as argument for avoiding initialization dependency - otherwise needs to care that self.repo is initialized above. But I'm not sure that it's a good way

Sorry, not sure I follow. Could you elaborate/rephrase, please?

I meant that needs first init self.repo like

def __init__(...): self.repo = ... self.checksum_jobs = ...

introduce complexity that self.checksum_jobs should be initialized after self.repo, so in terms of refactoring the order can change and it breaks the constructor

Well, but it will break in an obvious way with an attribute error, as self.repo won't be set. I would understand your logic if you would've made _get_checksum_jobs a static method or a helper function, but you've made it a full-blown method so using self.repo there would be totally fine. But current implementation works too. I guess another way to make it obvious would be to use _get_checksum_jobs(repo.config if repo else {}, config) 🙂But, again, these are non-vital details and neat-picking, so your call 🙂 Current implementation for this is mergeable.

efiop · 2020-01-14T09:03:19Z

dvc/remote/base.py

Could you please clarify what AttributeError is for?

there are places in tests that pass None or custom Repo() to RemoteBASE so such way I am checking that case.
https://github.com/JIoJIaJIu/dvc/blob/42c34d7269c24187a09bbebe0469647862a75fdd/tests/unit/remote/test_gdrive.py#L27

Why not if self.repo then? Attribute error might be too generic, as it will catch something that might happen deeper. Your first implementation had both if self.repo as well as except AttributeError, right? Let's keep only if self.repo then.

Why not if self.repo then

Cause here there is a repo but not config, https://github.com/JIoJIaJIu/dvc/blob/42c34d7269c24187a09bbebe0469647862a75fdd/tests/unit/remote/test_gdrive.py#L27
I didn't dare to touch the test
so I need to check self.repo and self.repo can be without config. I can change it to

self.repo.get('config', {})

Should I?

That specific test will still have repo.config though, because of https://github.com/iterative/dvc/blob/master/dvc/repo/__init__.py#L82 . Maybe some other test is not mocking it properly, then it should be fixed, unless it is unreasonably hard, of course 🙂

@JIoJIaJIu , I thought I had left a comment about AttributeError, but I couldn't find it.
Do you remember if I posted it? Am I going cuckoo 🙈 ?

@MrOutis #3133 (comment)

For avoiding checking config attribute existence I extended Repo class in GDrive tests
gurobokum@aec7781#diff-b780ac32bee8290565619b80d7051c85R18
Could you please double check that it doesn't break anything?

@MrOutis I've resolved it marking that the changes were applied. Is it ok or better to keep all threads open?

@JIoJIaJIu, it's fine to resolve them, I do the same) Just couldn't find it this time 😅

tests/func/test_remote.py

efiop · 2020-01-14T09:41:44Z

@JIoJIaJIu Looks good!

Good point about settings! Could you please create an issue for that?

tests/func/test_remote.py

dvc/remote/base.py

efiop

Looks great! Please see one minor comment above.

Close iterative#2920

efiop

Thanks!

ghost

thanks, @JIoJIaJIu !

efiop · 2020-01-15T21:37:48Z

@JIoJIaJIu Oops, looks like we forgot about docs. Could you please either create an issue in dvc.org or, even better, submit a PR to dvc.org?

gurobokum force-pushed the core_checksum branch from d9ce336 to 488bc3d Compare January 14, 2020 03:14

ghost reviewed Jan 14, 2020

View reviewed changes

gurobokum force-pushed the core_checksum branch from 488bc3d to 42c34d7 Compare January 14, 2020 08:33

efiop reviewed Jan 14, 2020

View reviewed changes

tests/func/test_remote.py Outdated Show resolved Hide resolved

ghost reviewed Jan 14, 2020

View reviewed changes

tests/func/test_remote.py Outdated Show resolved Hide resolved

gurobokum force-pushed the core_checksum branch from 42c34d7 to aec7781 Compare January 15, 2020 04:53

gurobokum requested review from a user and efiop January 15, 2020 05:02

gurobokum mentioned this pull request Jan 15, 2020

Rename config to settings in Remote classes #3153

Closed

efiop reviewed Jan 15, 2020

View reviewed changes

dvc/remote/base.py Outdated Show resolved Hide resolved

efiop suggested changes Jan 15, 2020

View reviewed changes

remote: config checksum_jobs

4918c56

Close iterative#2920

gurobokum force-pushed the core_checksum branch from aec7781 to 4918c56 Compare January 15, 2020 14:25

gurobokum requested a review from efiop January 15, 2020 14:26

efiop approved these changes Jan 15, 2020

View reviewed changes

ghost approved these changes Jan 15, 2020

View reviewed changes

efiop merged commit 6753e7b into iterative:master Jan 15, 2020

remote: config checksum_jobs #3133

remote: config checksum_jobs #3133

Uh oh!

Conversation

gurobokum commented Jan 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Additional details

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!

ghost Jan 14, 2020 • edited by ghost 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!

Uh oh!

ghost commented Jan 14, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gurobokum commented Jan 14, 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!

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!

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!

efiop commented Jan 14, 2020

Uh oh!

Uh oh!

Uh oh!

efiop left a comment

Choose a reason for hiding this comment

Uh oh!

efiop left a comment

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

efiop commented Jan 15, 2020

Uh oh!

Reviewers

gurobokum commented Jan 14, 2020 •

edited

Loading

ghost Jan 14, 2020 •

edited by ghost

Loading

ghost commented Jan 14, 2020 •

edited by ghost

Loading

gurobokum commented Jan 14, 2020 •

edited

Loading