Log results of requests.utils.super_len() when DANDI_DEVEL_INSTRUMENT_REQUESTS_SUPERLEN is set

[jwodder]

Part of #1257.

[codecov]

Codecov Report

Patch coverage: 80.00% and project coverage change: +0.03 🎉

Comparison is base (394b190) 89.02% compared to head (2afb540) 89.06%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1267      +/-   ##
==========================================
+ Coverage   89.02%   89.06%   +0.03%     
==========================================
  Files          76       76              
  Lines        9753     9767      +14     
==========================================
+ Hits         8683     8699      +16     
+ Misses       1070     1068       -2     

Flag	Coverage Δ
unittests	89.06% <80.00%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
dandi/upload.py	86.95% <80.00%> (-0.55%)	⬇️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

[yarikoptic]

nice design pattern on many aspects!

@asmacdo @candleindark - please review/take into your toolbelt on how to

• add optional context managers to the piece of code without duplication
• instrument/patch some functionality (outside or not) temporarily

[jwodder]

@yarikoptic I just realized that this approach will be a disaster, as Zarr entries are uploaded in a thread pool run within another set of threads for parallel asset uploads, yet monkeypatching is process-wide; hence, the patches to super_len() will all interfere with each other, with the most likely outcome involving the wrong filepaths being logged for super_len() return values.

[yarikoptic]

• don't patch within that method but rather "process wide" at e.g. import of dandi.upload stage. No need even to unpatch
• just log that o within super_len, no need for any exterior variables.

with this patch to requests

diff --git a/requests/utils.py b/requests/utils.py
index a367417f..dc4df067 100644
--- a/requests/utils.py
+++ b/requests/utils.py
@@ -131,6 +131,7 @@ def dict_to_sequence(d):
 
 
 def super_len(o):
+    print(f"super_len on str={o} repr={o!r}")
     total_length = None
     current_position = 0
 

I see

super_len on str=<_io.BufferedReader name='/home/yoh/.tmp/pytest-of-yoh/pytest-39/test_upload_zarr0/example.zarr/arr_0/0'> repr=<_io.BufferedReader name='/home/yoh/.tmp/pytest-of-yoh/pytest-39/test_upload_zarr0/example.zarr/arr_0/0'>

while running our tests -- so either of those (str or repr) would be good enough to identify the location.

this way patched function would need only the content which is given to it.

[yarikoptic]

somehow the run in that instrumented nfs environment resulted in

_________________ test_dandi_file_zarr_with_excluded_dotfiles __________________
dandi/tests/test_files.py:309: in test_dandi_file_zarr_with_excluded_dotfiles
    zf = dandi_file(zarr_path)
dandi/files/__init__.py:195: in dandi_file
    return factory(filepath, path, dandiset_path)
dandi/files/_private.py:71: in __call__
    return self.CLASSES[DandiFileType.classify(filepath)](
dandi/files/_private.py:42: in classify
    raise UnknownAssetError("Empty directories cannot be Zarr assets")
E   dandi.exceptions.UnknownAssetError: Empty directories cannot be Zarr assets

I didn't check if that is a flaky or whatnot, just restarting for a good check on that

[yarikoptic]

Ok, green now, let's proceed

[github-actions]

🚀 PR was released in 0.53.0 🚀