index: skip collection for fs with invalid fsid

[pmrowla]

related: iterative/dvc#10309

In dvcfs, trying to read fs.fsid forces us to load the erepo which may end up raising an SCMError in the event we cannot clone the source repo. In this case, we will not be able to collect anything from that fs, so we should just ignore trying to collect this data/storage

[codecov-commenter]

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (d89a3f2) 63.04% compared to head (03aea4b) 62.98%.

Files	Patch %	Lines
src/dvc_data/index/collect.py	25.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #510      +/-   ##
==========================================
- Coverage   63.04%   62.98%   -0.07%     
==========================================
  Files          62       62              
  Lines        4338     4341       +3     
  Branches      733      734       +1     
==========================================
- Hits         2735     2734       -1     
- Misses       1446     1449       +3     
- Partials      157      158       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pmrowla]

Even if we update dvcfs to re-raise the SCMError as an AttributeError instead when loading fs.fsid, this just pushes the problem further down the line. In this case we will still hit the same SCMError when trying to clone the repo again upon the next fs function call.

I'm not sure if catching the broad exception here is right though, we could re-raise ValueError in dvcfs (and then catch that here instead of BaseException)

cc @efiop

[efiop]

Yeah, BaseException indeed feels a bit too broad.

In this case we will still hit the same SCMError when trying to clone the repo again upon the next fs function call.

This might be fine, as we try to connect to check for auth errors anyway. Not sure if AttirubteError specifically is ideal though, maybe it is. Or BaseException might actually be alright to handle here like you did already, since it is about whatever fsid might throw at us... So sounds like I reach your coclusion as well here 😄 Though again error reporting is kinda odd...

[skshetry]

Should we still collect by falling back to data.fs.protocol?

[pmrowla]

In the general case it would probably be better to have fallback behavior, but for dvcfs/gitfs falling back here will just delay the failure to the next gitfs call (when it will retry cloning the repo and fail)