postprocess: move --exclude-file checking to after comment transfer but before comment checking#1555
postprocess: move --exclude-file checking to after comment transfer but before comment checking#1555kkysen wants to merge 21 commits intokkysen/postprocess-gc-cachefrom
--exclude-file checking to after comment transfer but before comment checking#1555Conversation
thedataking
left a comment
thedataking
left a comment
There was a problem hiding this comment.
I think this goes against the principle of least surprise. Exclude files and denylists generally mean "don't even attempt the operation here" but this PR changes the semantics of the exclude file to "try operation but accept failure and notify if operation succeeded". I don't think we should make that change.
kkysen
left a comment
kkysen
left a comment
There was a problem hiding this comment.
I think this goes against the principle of least surprise. Exclude files and denylists generally mean "don't even attempt the operation here" but this PR changes the semantics of the exclude file to "try operation but accept failure and notify if operation succeeded". I don't think we should make that change.
Hmm. I agree that this can definitely be surprising given the naming. What do you think about changing the exclude list to be like the xfail stuff we've used elsewhere a bunch (and documenting it more thoroughly)? Hopefully that would be clearer, and also let us have more useful behavior. You previously mentioned we might want to only exclude a function if it fails due to a certain reason, and xfail behavior would be much more conducive to that. If that doesn't seem useful, though, I can close this PR. We lose some nice behavior, but we can work around that manually.
This adds a newline that was missing in the prompt, so all the caches need to get updated, too.
…ude-file`/`--ident-filter`
With `--no-fail-fast`, failures are collected and displayed all at the end, which can be much easier to read.
… progress is easier to follow
…t used since the last `--gc-cache` call When there are input changes (like transpiler, refactorer, prompt, etc. changes), the cache becomes outdated and must be recalculated, but the previous entries aren't deleted. This tries to solve that. It tracks which cache entries are still actively tested/used (this is always done in `llm-cache/.gc`), and then `--gc-cache` deletes everything else. So the normal intended usage is to: * Run `rm -f llm-cache/.gc`. * Run all tests, updating the cache with new entries. * Run with `--gc-cache` to remove the outdated, unused entries.
…'s not actually used by it and comment transfering in general
Instead of storing paths in `.gc`, `.gc` is empty and just stores a ctime, before which everything should be deleted. When there's a cache hit, update the mtime, and use that to know which files are newer than the ctime and should be kept.
… but before comment checking This lets us do the comment transfer and cache the LLM response, but not error when checking the comments. This keeps the cache more stable (e.g., otherwise once you add failing fns to an exclude file, you have to delete them from the cache, or else it'll be out of sync). Also, this lets us see how changes affect the cache and if it no longer needs to be excluded.
This should warn us if excluding a fn is no longer necessary so that we can stop.
kkysen
force-pushed
the
kkysen/postprocess-gc-cache
branch
from
2612c20 to
ae20c1e
Compare
kkysen
force-pushed
the
kkysen/postprocess-exclude-late
branch
from
00e478b to
1868b14
Compare
kkysen
force-pushed
the
kkysen/postprocess-gc-cache
branch
5 times, most recently
from
3421a3c to
4c3b60a
Compare
kkysen
force-pushed
the
kkysen/postprocess-gc-cache
branch
from
8d1554c to
7a4ca7f
Compare
2 participants
This moves
--exclude-filechecking to after comment transfer, but before comment checking. This lets us do the comment transfer and cache the LLM response, but not error when checking the comments. This keeps the cache more stable (e.g., otherwise, once you add failing fns to an exclude file, you have to delete them from the cache, or else it'll be out of sync). Also, this lets us see how changes affect the cache and if it no longer needs to be excluded. And if a fn is excluded, but the comment transfer actually succeeds, there will be a warning so that we can stop excluding it.