[WIP][Showcase] dvc: use dynamic scope to stop callback/flag passing

[Suor]

The idea is to simplify function signatures and make intermediate calls not need to know when some high-level code is talking some low-level one.

Showcased on annoying enough no_progress_bar and progress_callback. Might be also used to simplify:

• combining pbars
• jobs
• force
• other cli or config options only low level code needs to know about

[ghost]

👍

[ghost]

woah, this is clever 👌

[ghost]

Is this indented correctly?

[ghost]

Isn't this the same as doing?

def __getattr__(self, name):
    pass

[ghost]

@Suor , what about using this instead?

from contextlib import contextmanager
from collections import UserDict


class ContextFlags(UserDict):
    def __getattr__(self, name):
        return self.data.get(name)

    @contextmanager
    def __call__(self, **flags):
        self.data.update(flags)

        yield

        for key in flags.keys():
            del self.data[key]

[ghost]

I like the idea 👍

[Suor]

@efiop @pared may you take a look, guys?

@MrOutis let's discuss the approach first, not the implementation details.

[ghost]

@MrOutis let's discuss the approach first, not the implementation details.

All right, @Suor!

As I mentioned before, I like the approach. It reminds me a bit of Click's contexts

So far, we've been trying to explicitly pass all the state/arguments needed for the execution (dependency injection, if you want to use a fancy name). I emphasize trying because we also rely on some external state (i.e. environment variables, configuration).

The downside, is that having several nested calls require us to pass those arguments all the way down, for example (--jobs like @Suor mentioned):

CmdDataPull -> Repo.pull(**self.args) -> ._fetch(**self.args) -> .used_cache(**self.args) -> .cloud.pull(**self.args) -> .cloud.repo.cache.local.pull(**self.args) -> remote._process(**self.args) -> self.status(**self.args) -> remote._download.

I'd be more in favor of relying less on nesting calls and flatting the class hierarchy, but this is unlikely to happen soon (and would likely bring more pain than joy in the process -- heated discussions / holy wars). Introducing ContextFlags seems like a step towards flat land, so I'm on board. Not sure where this is going to lead us 😅

[shcheklein]

Interesting stuff! A few quick questions:

• Intuitively looks like noop is not needed? pbar or noop looks ugly. Would really prefer to flags.name.do.smt - one line.
• Would love to see a bit more complicated example - managing outputs, multiple progress bars - when one is needed and another one is not. Just curios how would it start looking on the very top level, how would wrappers for managing outputs look like?
• Natural follow up from the previous question - would it (do we need to?) support nesting (a stack of contexts)?

[pared]

@Suor sorry for the late response
I am all for that approach, passing down flags is cumbersome. I am trying to come up with a reason why this approach could be worse than flags, no idea so far.
Using with flags(tqdm_disable=True) seems to be elegant and easily readable, no need to "mentaly parse" and keep track of all arguments passed down to a particular method.

[Suor]

@shcheklein

If you don't specify a noop in the usage place then some noop class should be set somewhere higher. The thing here is where do we decide what should be a behavior when there is no pbar, the answers might be:

• globally
• scoped
• a usage place

If we decide at usage place, say method, e.g. .checkout(), should just work outputting nothing if there is no pbar set, while other method, e.g .download() should raise an error then that logic should be right there. Alternatively we can use separate flags/pbars for that, which will have some global defaults:

flags.checkout_pbar.update_desc()  # is a noop until set
flags.download_pbar.update(...)  # raises unless set

Would love to see a bit more complicated example

The general idea is passing some info to all temporally scoped code without passing it explicitly, i.e. allowing higher level code speak with lower level one without everyone in the middle passing that. To use that for combining pbars we way use:

# This says to any nesting pbars to be combined into this
with Tqdm(..., combine="download") as pbar:
    # ... use it as is

# This behaves as usual unless under combine="download"
# When under that it updates that progress instead.
with Tqdm(..., action="download") as pbar:
    # ... use it as is

There would need to be some trickery to update totals and/or desc.

would it (do we need to?) support nesting

It already does

[Suor]

Combining pbars might be implemented with explicit passing too though, which we might want to go after first.

[efiop]

Maybe it would make sense to just have pbar as a part of flags or some other structure instead of doing flags.tqdm or noop everywhere?

[efiop]

Sorry for an extremely late response from me, but this looks pretty nice.

[dmpetrov]

Sorry for an extremely late response from me, but this looks pretty nice.

:)
@efiop @Suor should we finalize and merge this?

[efiop]

Closing as stale, maybe will come back to it in the future.