Document program architecture #166

jwodder · 2024-07-13T15:31:52Z

Part of #148.

codecov · 2024-07-13T15:33:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.30%. Comparing base (8d058fe) to head (4b8db19).
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #166      +/-   ##
==========================================
+ Coverage   47.08%   47.30%   +0.21%     
==========================================
  Files          27       27              
  Lines        3859     3854       -5     
==========================================
+ Hits         1817     1823       +6     
+ Misses       2042     2031      -11

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

jwodder · 2024-07-15T11:58:22Z

@yarikoptic I feel that this could use more details, but I'm not sure what. What do you want/need to see in this document?

jwodder · 2024-07-25T12:58:30Z

@yarikoptic Ping.

yarikoptic

I think this is very nice and informative. Left a few more questions to clarify. Follow up with more details on zarr man could be done separately.

yarikoptic · 2024-07-25T19:31:46Z

doc/architecture.md

+> A new architecture is currently being planned for the code.  See [issue
+> #67](https://github.com/dandi/dandidav/issues/67) for more information.
+
+This document is best augmented by the doc comments in the `dandidav` source.


I am not entirely sure that "doc comments" are in this context... so it should not be a PR changing this file?

Doc comments are the documentation for the code's API, embedded in the source as triple-slashed comments (example) — like Python's docstrings, but using comments instead.

Also, by "augmented," I meant "To get more information about dandidav, read the documentation in the source," not "If you want to add to this document, add to the documentation in the source instead."

I've reworded this section in f8355a5.

yarikoptic · 2024-07-25T19:33:38Z

doc/architecture.md

+following inside a clone of this repository:
+
+```console
+cargo doc --open --document-private-items --no-deps


just to see comments involved installing everything but kitchensink (over 300 modules) and 35 seconds... and then opened

which lacks any "Overview" or architecture as far as I see it, not to mention "doc comments". Was this document forgotten to be included?

You're looking at (the top page of) the rendered documentation for the source code. It's not supposed to contain an overview or dedicated architecture description (that's what this document is for). The descriptions shown for each item are the rendered doc comments.

doc/architecture.md

yarikoptic · 2024-07-25T19:42:39Z

doc/architecture.md

+      present in all other responses.
+
+- If any error occurs during the processing of a request, it will almost always
+  "bubble up" to `DandiDav::handle_request()`, which will log the error and


Suggested change

"bubble up" to `DandiDav::handle_request()`, which will log the error and

"bubble up" to [`DandiDav::handle_request()`][handle-request], which will log the error and

for convenience since we already have this hyperlink

Done in 4b8db19.

doc/architecture.md

yarikoptic · 2024-07-25T20:43:32Z

doc/architecture.md

+the mirror at <https://datasets.datalad.org/dandi/zarr-manifests/>.  It is the
+data source for the `/zarrs/` hierarchy served by `dandidav`.
+
+See the documentation for the `dandidav::zarrman` module for more information.


it is not really descriptive there ATM. Would be nice to get (follow up PR) some brief similar description for operations on zarrs.

Expanded in bb682a1.

yarikoptic · 2024-07-25T20:45:57Z

doc/architecture.md

+
+    - Making calls to a `DandiClient` or `ZarrManClient` instance as
+      appropriate to fetch information about the given resource and possibly
+      its child resources


IIRC our endpoints for under zarr operations under /dandisets/ as within the .zarr assets were notably slower than operation on zarrs under /zarrs/. Is there any caching done/worth doing here to avoid redoing some splitting/decision making?

There is no caching at the moment, but see #6.

yarikoptic · 2024-07-25T20:49:19Z

doc/architecture.md

+      redirect to the Archive API download URL.  Unfortunately, certain WebDAV
+      clients (i.e., [davfs2](https://savannah.nongnu.org/bugs/?65376)) do not
+      support WebDAV servers that return redirects to other redirects, so the
+      `--prefer-s3-redirects` CLI option was added to `dandidav` to instead


is that the option we use on our https://webdav.dandiarchive.org/ ? worth making explicit note here.

Done in 4b8db19.

yarikoptic · 2024-07-25T20:50:16Z

doc/architecture.md

+      usually the blob ID and not user-friendly.  Hence, by default, a `GET`
+      request to `dandidav` for a blob asset will be responded to with a
+      redirect to the Archive API download URL.  Unfortunately, certain WebDAV
+      clients (i.e., [davfs2](https://savannah.nongnu.org/bugs/?65376)) do not


isn't there also performance benefit from going directly to S3 in case of heavy (non-zarr) filetree hierarchies?

I doubt there's much benefit. The Archive download URLs refer to assets by ID rather than by path, so it should be a simple lookup on the Archive's end. The cost to dandidav is the same regardless of which download URL type it's returning.

doc/architecture.md

Also, the `--document-private-items` option is already implied for binaries.

jwodder · 2024-08-12T12:24:04Z

@yarikoptic Ping.

yarikoptic · 2024-08-12T16:01:50Z

I have skimmed briefly through changes etc, IMHO it is worth merging and then if anything to extend/fixup - we could do it later.

jwodder added documentation Improvements or additions to documentation skip deployment Do not deploy this PR upon merge labels Jul 13, 2024

jwodder mentioned this pull request Jul 13, 2024

Add developer-facing documentation #148

Closed

4 tasks

jwodder mentioned this pull request Jul 13, 2024

Add doc comments to all items #32

Open

12 tasks

jwodder force-pushed the archdoc branch from 27703d2 to b287146 Compare July 14, 2024 12:19

jwodder changed the title ~~Document architecture and a good portion of the code~~ Document program architecture Jul 14, 2024

jwodder force-pushed the archdoc branch 2 times, most recently from f3df216 to 5e7c8a4 Compare July 14, 2024 12:25

jwodder modified the milestone: Current priorities Jul 17, 2024

yarikoptic reviewed Jul 25, 2024

View reviewed changes

jwodder force-pushed the archdoc branch from bb682a1 to 10252e0 Compare July 26, 2024 15:39

jwodder requested a review from yarikoptic August 6, 2024 13:58

jwodder added 6 commits August 8, 2024 11:15

[skip ci] Document program architecture

3a7e3ca

Mention cargo doc command for browsing documentation

e3784f3

Updates based on review comments

20bffc7

Update for #175

4809dcc

[skip ci] Expand ZarrManClient documentation

00d6968

[skip ci] Improve reference to doc comments

f8355a5

Also, the `--document-private-items` option is already implied for binaries.

jwodder force-pushed the archdoc branch from 10252e0 to f8355a5 Compare August 8, 2024 15:18

jwodder marked this pull request as ready for review August 12, 2024 16:13

jwodder merged commit e82610c into main Aug 12, 2024

jwodder deleted the archdoc branch August 12, 2024 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document program architecture #166

Document program architecture #166

jwodder commented Jul 13, 2024 •

edited

Loading

codecov bot commented Jul 13, 2024 •

edited

Loading

jwodder commented Jul 15, 2024

jwodder commented Jul 25, 2024

yarikoptic left a comment

yarikoptic Jul 25, 2024

jwodder Jul 26, 2024

jwodder Aug 8, 2024

yarikoptic Jul 25, 2024

jwodder Jul 26, 2024

yarikoptic Jul 25, 2024

jwodder Jul 26, 2024

yarikoptic Jul 25, 2024

jwodder Jul 26, 2024

yarikoptic Jul 25, 2024

jwodder Jul 26, 2024

yarikoptic Jul 25, 2024

jwodder Jul 26, 2024

yarikoptic Jul 25, 2024

jwodder Jul 26, 2024

jwodder commented Aug 12, 2024

yarikoptic commented Aug 12, 2024

	"bubble up" to `DandiDav::handle_request()`, which will log the error and
	"bubble up" to [`DandiDav::handle_request()`][handle-request], which will log the error and

Document program architecture #166

Document program architecture #166

Conversation

jwodder commented Jul 13, 2024 • edited Loading

codecov bot commented Jul 13, 2024 • edited Loading

Codecov Report

jwodder commented Jul 15, 2024

jwodder commented Jul 25, 2024

yarikoptic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwodder commented Aug 12, 2024

yarikoptic commented Aug 12, 2024

jwodder commented Jul 13, 2024 •

edited

Loading

codecov bot commented Jul 13, 2024 •

edited

Loading