Type error on FileContent results in infinite loop

[hopper-signifyd]

Describe the bug
I'm working on a custom plugin and I did this:

content = FileContent(path=versions_file_path, content="some string")
digest = await Get(Digest, CreateDigest([content]))

That resulted in the infamous Filesystem changed during run: retrying 'MyGoal' infinite loop. I was really confused. Upon further inspection of the run logs, I found this:

09:14:56.95 [31m[ERROR][0m panic at 'called `Result::unwrap()` on an `Err` value: "Field `content` was not convertible to type alloc::vec::Vec<u8>: PyErr { type: <class 'TypeError'>, value: TypeError(\"'str' object cannot be interpreted as an integer\"), traceback: None }"', src/intrinsics.rs:422

So I updated the code above to call .encode() on the string before passing it as content and things started working. However, I feel like my mistake should have resulted in an error / failure instead of the infinite Filesystem changed loop as this just made things harder to debug.

Pants version
2.16.0

OS
MacOS

Additional info
I like pants

[benjyw]

Good catch @hopper-signifyd . We'd welcome a PR to detect the wrong arg type in the FileContent ctor and error sensibly.

[krishnan-chandra]

I ran into this issue myself, so I made a PR to fix it in #20261.

[hopper-signifyd]

Thank you!

[jsirois]

@hopper-signifyd, @benjyw and @krishnan-chandra - is it not the case that Pants leverages Python types and build time typechecking? See my comment here: https://github.com/pantsbuild/pants/pull/20261/files#r1417025856

To add runtime type checks here and there (or even everywhere) seems a path to madness. Instead perhaps the plugin developer docs should be improved to point out the criticality of build time type checking.

[benjyw]

Pants does, but plugins may not. I wouldn't advocate adding runtime type checks in most cases, but this particular case seems to have bitten multiple people, and leads to a hard-to-debug and unintuitive infinite loop. I think the tradeoff in this case is beneficial.

[benjyw]

(Adding typechecking to plugin code is desirable in general, but I don't know if it's practical for plugin authors to do so in all cases.)

[stuhood]

The framework that you are using should be checking types generically and automatically, and pyo3 generally does. So I agree with John there.

The reason that the failure mode here is so unfortunate (failing in a loop) is because the Rust code is using unwrap and panicing, rather than propagating the actual error (which should actually be very usable, due to pyo3).

So rather than adding individual checks like this, the patches from #13608 represent a more general solution to this problem: #18233, #18370. More of those patches would be great.

[stuhood]

So to be clear: when we see issues like this one (#20024), we should be linking them to #13608 ... and/or asking folks whether they have time to continue to push on that one.

[benjyw]

That is excellent context that I sadly didn't have in my mental model.

I'm happy to either revert #20261 or keep it as a band-aid until the more robust fix is in.

[benjyw]

@krishnan-chandra do you want to tackle a patch similar to #18233, #18370 that would cover this case?

[krishnan-chandra]

Sure! I need to do some reading on Rust first, but I'm happy to contribute towards a better solution here - hopefully I can tackle this specific part of the Rust code that covers FileContent.

[benjyw]

Thanks!

To sum up - this was my lack of depth, not @krishnan-chandra's, who was implementing my band-aid suggestion.