import: don't forget to set cache.dir

[efiop]

Fixes #9385

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (ad39bf1) 91.58% compared to head (f89a6ff) 91.59%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9415   +/-   ##
=======================================
  Coverage   91.58%   91.59%           
=======================================
  Files         487      487           
  Lines       37775    37779    +4     
  Branches     5436     5436           
=======================================
+ Hits        34598    34602    +4     
  Misses       2620     2620           
  Partials      557      557           

Impacted Files	Coverage Δ
dvc/dependency/repo.py	100.00% <ø> (ø)
dvc/repo/open_repo.py	82.38% <100.00%> (+0.33%)	⬆️
tests/unit/stage/test_stage.py	100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[shcheklein]

Thanks a lot for fixing it so quick. My 2cs - we should have a test for this since it was an unpleasant regression after refactoring. Interfaces are not obvious internally (via kwargs and layers of abstractions).

[efiop]

@shcheklein I disagree, this should work by design. This is the only place for DVCFileSystem. A test would be complex and highly specific, that is a bad kind of test.

Interfaces are not obvious internally (via kwargs and layers of abstractions).

I agree that dvcfs could have the args cleaned up to make it more obvious. But config was an introduced by design specifically for these kinds of cases. This is much clearer than external_repo mess we had before.

[shcheklein]

I didn't say anything about how that test should look like.

• Tests are here to insure that things "by design" keep working as designed.
• this should work by design - but it wasn't
• This is the only place for DVCFileSystem - that's fine. If it's tested there, here it can be as simple as:

def test_that_we_enable_cache():
    # this is needed for the underlying DataFs try cache first
    assert cache is passed to DVCFs

• a similar one for repo.

Any combination like this already solves 80% of the intention:

• It provides a documentation
• It declares the expectation on behavior, etc

In this specific situation, I think it's wort having a functional/integration test still, or there should be a benchmark that detects this. Bottom line- we should not be testing this on users.

I agree that dvcfs could have the args cleaned up to make it more obvious.

It might be better now. I was merely saying that the way it's written now looks fragile enough to break it next time.

[efiop]

@shcheklein Created #9417