dvc: refactor config by Suor · Pull Request #3298 · treeverse/dvc

Suor · 2020-02-10T15:54:42Z

Advantages:

read like a dict
modify as a dict for testing or manipulation purposes

wrap modifications into context manager to save them:

with config.edit("local") as conf:
    conf["core"]["remote"] = "new-default"

Design decisions:

dropped string constants
remote sections are parsed into a simple subdict upon load
remote config schema is now defined by url scheme
relative paths are handled transparently upon load/save
user config and remote manipulations are moved to commands
same dev manipulations are done by simply modifying a config dict

efiop · 2020-02-10T16:28:25Z

@Suor Looks amazing! Please check the tests though, looks like slight hickup with schemas for some remotes.

Suor · 2020-02-10T16:32:17Z

@efiop I see two issues:

setting type for hdfs remote fails, which makes sense, tests need to be adjusted
windows paths not being recognized sometimes by validation

I will fix these tomorrow.

efiop · 2020-02-11T17:45:15Z

@Suor Please rebase.

Suor · 2020-02-11T18:42:56Z

@efiop rebased

pared

One question

pared · 2020-02-12T13:04:35Z

Shouldn't here be config.get("sse","")? And if not, why do we do config.get("acl","")?

Doesn't matter really until it's boolean false it won't be used anyway.

Removed "" for consistency.

efiop · 2020-02-13T03:40:24Z

@Suor You might've lost _cwd logic, which was resolving relative paths based on config location. Or am I missing something here?

I thought that we have test for this 🤔

This is not needed anymore:

relative paths are handled transparently upon load/save

@Suor Could you please refer me to some part of the code?

Nevermind, I see it now.

Advantages: - read like a dict - modify as a dict for testing or manipulation purposes - wrap modifications into context manager to save them: with config.edit("local") as conf: conf["core"]["remote"] = "new-default" Design decisions: - dropped string constants - remote sections are parsed into a simple subdict upon load - remote config schema is now defined by url scheme - relative paths are handled transparently upon load/save - user config and remote manipulations are moved to commands - same dev manipulations are done by simply modifying a config dict

efiop · 2020-02-13T16:44:18Z

@Suor Please check the DS and CC reports, there are some good points there.

efiop · 2020-02-13T16:45:27Z

+        def resolve(path):
+            if os.path.isabs(path) or re.match(r"\w+://", path):
+                return path
+            return RelPath(os.path.join(cwd, path))


cwd is defined after this helper in which it is used. This is error-prone.

This is minor. Nothing functionally wrong about it because of the same namespace, but causes a mental hickup.

Defining subfunction in the middle causes mental hiccup too)

I can move it if that worries you.

Suor · 2020-02-13T16:57:34Z

@efiop I actually looked them through a couple of days ago) Most of them not introduced by this PR, from the rest:

ByUrl complexity here - splitting it is not a good idea
not calling dict.__init__() here - Config has very different constructor signature.
self in .validate() here and if/elif here just nitpicks, can't cause any issues
get_dir() here protected by an assert.

While some stylistic changes can be done, that will bring no value. Diving into DS was a waste of time again.

efiop · 2020-02-13T17:03:44Z

@Suor

ByUrl complexity here - splitting it is not a good idea

👍

not calling dict.init() here - Config has very different constructor signature.

👍

self in .validate() here and if/elif here just nitpicks, can't cause any issues

These are worth fixing, it affects the code style, we avoid such things in other places.

get_dir() here protected by an assert.

👍

While some stylistic changes can be done, that will bring no value. Diving into DS was a waste of time again.

Style is not a waste of time for DVC, please fix those few issues left.

efiop · 2020-02-13T17:35:46Z

Thanks @Suor ! 🙏

Suor requested a review from efiop February 10, 2020 15:54

Suor force-pushed the config branch from 367e338 to a6a486c Compare February 11, 2020 09:18

efiop added refactoring Factoring and re-factoring p1-important Important, aka current backlog of things to do labels Feb 11, 2020

efiop assigned Suor Feb 11, 2020

Suor force-pushed the config branch from 52404ad to 9eda1b1 Compare February 11, 2020 18:42

pared reviewed Feb 12, 2020

View reviewed changes

efiop reviewed Feb 13, 2020

View reviewed changes

Suor added 4 commits February 13, 2020 23:41

test: do not set cache.type for remotes that doesn't support it

c1f0cfe

dvc: fix windows absolute paths validation in config

7356c1d

test: fix windows separate drive test

a07812b

Suor force-pushed the config branch from 9eda1b1 to a07812b Compare February 13, 2020 16:42

efiop reviewed Feb 13, 2020

View reviewed changes

dvc: stylistic fixes in config

e8159b6

efiop merged commit 6ccae71 into treeverse:master Feb 13, 2020

pared mentioned this pull request Feb 14, 2020

daemon config fix #3324

Merged

3 tasks

weekly-digest Bot mentioned this pull request Feb 16, 2020

Weekly Digest (9 February, 2020 - 16 February, 2020) #3342

Closed

efiop mentioned this pull request Sep 10, 2020

config: deep remote:// notation is broken #4556

Closed

skshetry mentioned this pull request Dec 26, 2022

config.get(section, opt) doesnt coerce value to appropriate type #3154

Closed

Conversation

Suor commented Feb 10, 2020

Uh oh!

efiop commented Feb 10, 2020

Uh oh!

Suor commented Feb 10, 2020

Uh oh!

efiop commented Feb 11, 2020

Uh oh!

Suor commented Feb 11, 2020

Uh oh!

pared left a comment

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!

efiop commented Feb 13, 2020

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!

Suor commented Feb 13, 2020

Uh oh!

efiop commented Feb 13, 2020

Uh oh!

efiop commented Feb 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants