How should hflow raise FileNotFoundError?

[chiruu12]

#95 moved four raise sites to the three-argument errno form and left two questions
open. @kstonekuan asked for them here rather than in a review, since the second one
changes an exception type on a public API.

Where the tree stands on main (2f07aa0), there are three forms in use.

Three-argument errno form (4 sites). storage.py:656, _bundle.py:861 and :895,
_deploy.py:153 and :168.

raise FileNotFoundError(errno.ENOENT, os.strerror(errno.ENOENT), str(path))

str(error) is [Errno 2] No such file or directory: '/x/y.py'. error.filename
is set, so a caller can branch on the path without parsing the message.

Explanatory message string (6 sites). app.py:211, :802, :1093, :1101,
curation.py:140, _bundle.py:823.

raise FileNotFoundError(
    f"no rendered bundle at {bundle_directory} (missing {required_file.name}); "
    "run `hflow up` (or render_bundle) first"
)

str(error) is that sentence. error.filename is None and errno is None.

Bare path (2 sites). storage.py:344, testing.py:514.

raise FileNotFoundError(local_file)

str(error) is the path and nothing else. This is the one #26 was filed about.

The forms are not equally defensible, and I do not think the answer is "pick one".

The explanatory sites are the good case. Each says what was looked for and what to do
next, and none of them is a plain "this path is missing": the bundle one means up was
never run, the curation.py one means the location is not a catalog root. Collapsing
those into [Errno 2] would lose the only useful part. What they do give up is
error.filename, which nothing in the tree reads today.

The bare sites are the bad case, and there is a concrete symptom. StorageRoot.fetch
has two implementations that answer the same failure differently:

• LocalStorageRoot.fetch (storage.py:344) raises the bare path.
• BucketStorageRoot.fetch (storage.py:510) does not raise at all: obstore.head
  raises, so the text is obstore's and the repo does not control it.
• fetch_uri (storage.py:656), one screen down and named for the same job, uses the
  errno form.

So the same missing object produces a different message depending on which root the
data path resolved to. That is worth fixing whichever way the convention lands.

Question 1. Is the rule "errno form whenever there is nothing to add, message string
when there is"? If so the two bare sites move to errno, the six explanatory sites stay,
and the interesting part is whether the explanatory ones should also set filename,
which needs the errno form plus a message and is not one call:

error = FileNotFoundError(errno.ENOENT, "no rendered bundle ... run `hflow up` first", str(path))

That still prints [Errno 2] no rendered bundle ..., so the prefix arrives whether or
not it helps. A small _missing(path, why=None) helper in one module would make the
choice once. I do not know if that is worth a helper for eight sites.

Question 2. --pipeline <a-directory> currently reports "No such file or directory"
about a path that exists, because every site tests is_file() and raises ENOENT on the
false branch. The accurate error is IsADirectoryError (EISDIR).

IsADirectoryError is a subclass of OSError, not of FileNotFoundError, so any
except FileNotFoundError around these calls stops catching it. In the tree that is
cli.py:467, :525, :566, :588, :632, :684, :698, :710, plus app.py:1092
and _compose.py:49. Every one of those would need widening to OSError or to a tuple,
and downstream code outside this repo would break in a way that only shows up on the
directory input, which is exactly the input nobody tests.

The cheap version keeps the type and fixes only the text:

if path.is_dir():
    raise FileNotFoundError(errno.EISDIR, os.strerror(errno.EISDIR), str(path))

str(error) is then [Errno 21] Is a directory: '/x/y', which is accurate, while the
class stays FileNotFoundError, which is a lie about the class but keeps every existing
handler working. I lean this way for now and would rather raise the honest type in a
release that is already breaking something else.

Happy to send a PR for whichever shape you want, or to leave it if the answer is that
eight sites is not enough to justify a convention.

[chiruu12]

Filed the two concrete cases so they are not lost while this is open, and so nobody picks one up before it is settled:

• storage: LocalStorageRoot.fetch raises a bare path where fetch_uri raises errno #101 for question 1, the bare path in LocalStorageRoot.fetch against the errno form in fetch_uri
• cli: --pipeline pointed at a directory reports 'No such file or directory' #102 for question 2, --pipeline on a directory

Both say on their face that they are not up for grabs and point back here. Once there is an answer they are small changes and anyone can take them.

[kstonekuan]

Both settled. Thank you for laying the three forms out with the sites, and for refusing the obvious answer: "pick one form" was what I would have said before reading this, and you are right that it makes six sites worse.

Question 1: yes, that is the rule. Errno form when there is nothing to add, message string when there is. The two bare sites are the only ones that move.

Two calls on the details you raised:

No helper. Eight sites do not earn _missing(path, why=None), and the composed form argues against itself: [Errno 2] no rendered bundle at ... run hflow up first reads worse than either half alone. The errno prefix is useful exactly when the message is only the path, and noise when a real sentence follows it.

Do not retrofit filename onto the explanatory sites. You noted nothing in the tree reads it. Setting it everywhere in case something might is speculative surface, and it forces the three-argument call at every site that currently has a clean one-argument raise. If a caller ever needs to branch on the path, that is the moment to add it, at the site that needs it.

So the rule to write down: a bare FileNotFoundError(path) is never correct. Either the path is the whole story, in which case use the errno form so str() says why, or it is not, in which case say what was looked for and what to do next.

Question 2: take the cheap version. FileNotFoundError(errno.EISDIR, os.strerror(errno.EISDIR), str(path)), giving [Errno 21] Is a directory: '/x/y'. Accurate text, and every existing handler keeps working.

IsADirectoryError is the honest type and I want it eventually. The reason not to now is the one you identified: it breaks ten in-tree except FileNotFoundError sites plus anything downstream, and only on the input nobody tests, which is the worst possible failure signature. Pre-v1 with no users is a reasonable time to take a break like that, and I considered it. What tips it is that this change buys a more correct exception class and nothing else: no caller can currently act on the distinction, so we would be paying the compatibility cost for a property no one can observe yet. Worth doing when something else is already breaking, or when a caller genuinely needs to branch on it.

Worth putting a comment at those sites saying the class is deliberately wrong and why, so the next reader does not "fix" it without knowing the cost.

Unblocking #101 and #102 now, both yours if you want them. #101 is the bare-to-errno move at storage.py:344 and testing.py:514. #102 is the is_dir() branch at the four sites. They are independent, so either order.

One note on #101 while you are in there: BucketStorageRoot.fetch letting obstore's own error through is fine and not part of this. We do not control that text and should not wrap it to look like ours.