1978945 - [mach-try-perf] "Error: Revision `yuszunqrpkqu` doesn't exist" when invoking ./mach try perf in a jujutsu repository

Status: RESOLVED FIXED

(Testing :: Performance, defect)

Description

[Markus Stange [:mstange]]

Attachment: error log

Bug 1929372 recently added preliminary support for the Jujutsu version control system. But it currently doesn't seem to work with ./mach try perf.

See the attached log for the error.

Comment 1

[Kash Shampur [:kshampur] ⌚EST]

(I am not using JJ actively so can't comment too much on the jj quirks/incompatibility and such) but I tried this out on another checkout with jj setup, and at first I did hit this error described by :mstange
However a repeat of ./mach try perf seemed to push just fine with no error... not sure why.
e.g. these Try pushes are a result of this scenario:

Base revision's try run: https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=143958
Local revision's try run: https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=143960

Thought this would be worth making note of for now

Comment 2

[Greg Mierzwinski [:sparky]]

This seems like an issue in mozversioncontrol, not mach try perf. It looks like updating to the original/head revision is what's breaking here, and we're getting that revision from here: https://searchfox.org/mozilla-central/source/tools/tryselect/selectors/compare.py#38-41

In the JJ adapter, the branch is None, so we use this for the current_revision_ref: https://searchfox.org/mozilla-central/source/python/mozversioncontrol/mozversioncontrol/repo/jj.py#104-110

I see there's a warning there about not using it directly without also calling the resolve_to_commit method. Though I'm not a fan of having to make JJ-specific code in mach try perf now :/

It also makes me wonder what the purpose of this head_ref property is if we shouldn't/can't use it. :sfink, I see that you added this code - is there anyway we could make the head_ref usable without having to call additional methods?

Comment 3

[Steve Fink [:sfink] [:s:]]

(In reply to Greg Mierzwinski [:sparky] from comment #2)

This seems like an issue in mozversioncontrol, not mach try perf. It looks like updating to the original/head revision is what's breaking here, and we're getting that revision from here: https://searchfox.org/mozilla-central/source/tools/tryselect/selectors/compare.py#38-41

In the JJ adapter, the branch is None, so we use this for the current_revision_ref: https://searchfox.org/mozilla-central/source/python/mozversioncontrol/mozversioncontrol/repo/jj.py#104-110

Thanks for tracking down where this stuff is happening.

I see there's a warning there about not using it directly without also calling the resolve_to_commit method. Though I'm not a fan of having to make JJ-specific code in mach try perf now :/

Yes, please don't!

It also makes me wonder what the purpose of this head_ref property is if we shouldn't/can't use it. :sfink, I see that you added this code - is there anyway we could make the head_ref usable without having to call additional methods?

I didn't explain it very well in the comment. I was basically trying to detect subtle mismatches when using JJ with code that expects git. JJ will normally be working with change ids rather than git commit ids (hashes), and it is expected that the git hash for a change id will be different over time. If a JJ change is converted to the current git commit id that it corresponds to, then it would be easy for code to accidentally continue to use an old version of the change. (It would still "work", since that git commit still exists, but it might not have the right content anymore.)

The mozversioncontrol.repository API layer is supposed to provide all the operations you need in a vcs-agnostic way. Which means that if you pass the head_ref into something that assumes git, then you're not going through that layer. Which implies you're trying to do something that isn't supported, and we should support it rather than having git-specific code scattered around the tree.

So the idea is definitely not to add a call to _resolve_to_commit. It's instead to look at what the code is doing with the head_ref and make it go through a generic repository action instead. If the JJ change id ever needs to be converted to a git commit id in order to do something with it, then that should happen inside jj.py. (And most operations should never do that since native jj operations work with the change ids anyway.)

...which all boils down to: why doesn't vcs.update(current_revision_ref) Just Work? All of that babble about needing to go through the vcs layer is already happening! In fact, it's failing when running jj new yuszunqrpkqu, which is very much a jj command running with a jj change id. Erm... my only guess is that this is an artifact of creating a temporary commit for pushing to try, that commit is being used as the head_ref, then it's discarded and still used. Though that shouldn't be different than with git...

Thankfully, I can reproduce locally, and in fact the jj op log shows me the full history of what happened. Ugh, I see what's going on. It is the temporary commit, which JJ obliterates locally through some code that is perhaps excessively clever. For mach try perf, in particular, we need to refer to the tip of the try push, and JJ no longer has a local change corresponding to that. It still has the commit for the code that was pushed without the try_task_config.json stuff. Ok, I need to think about how to fix this... (this is all well within the JJ support code that I wrote; I'll figure something out.)

---

I hope someone enjoys the novel I've written above.

Comment 4

[Steve Fink [:sfink] [:s:]]

(In reply to Kash Shampur [:kshampur] ⌚EST from comment #1)

(I am not using JJ actively so can't comment too much on the jj quirks/incompatibility and such) but I tried this out on another checkout with jj setup, and at first I did hit this error described by :mstange
However a repeat of ./mach try perf seemed to push just fine with no error... not sure why.

Yeah, I got that too. It's because when it crashes out, it leaves the base revision checked out. So it's comparing the base rev to itself.

e.g. these Try pushes are a result of this scenario:

Base revision's try run: https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=143958
Local revision's try run: https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=143960

Yep, both redirect to https://treeherder.mozilla.org/jobs?repo=try&revision=6765a2d660affa9d590daddf27b65c7bcde9462f

(Thanks for mentioning it, though. I was way less confused about what was going on knowing that you'd seen it too; I knew it wasn't just something I had done.)

Comment 5

[Greg Mierzwinski [:sparky]]

(In reply to Steve Fink [:sfink] [:s:] from comment #3)

Thankfully, I can reproduce locally, and in fact the jj op log shows me the full history of what happened. Ugh, I see what's going on. It is the temporary commit, which JJ obliterates locally through some code that is perhaps excessively clever. For mach try perf, in particular, we need to refer to the tip of the try push, and JJ no longer has a local change corresponding to that. It still has the commit for the code that was pushed without the try_task_config.json stuff. Ok, I need to think about how to fix this... (this is all well within the JJ support code that I wrote; I'll figure something out.)

---

I hope someone enjoys the novel I've written above.

I did, thanks for providing all the context behind this decision :)
Let us know if you need any help or some fixes in mach try perf for this.

Comment 6

[Steve Fink [:sfink] [:s:]]

I'll write out the problem because I love to hear myself talk.

The problem is that mach try perf has to update to various different commits (in order to push the base revision as well as the current revision). With JJ, vcs.update actually makes a new working copy commit on top of the requested commit, because I made everything follow the "squash workflow" as much as possible. The problem is that empty working copy commits are discardable and jj discards them whenever they're no longer needed (ie, they're empty, have no description or bookmarks, and you're not using them for a working copy). So when grabbing the current_revision_ref at the beginning of the whole process, it gets such an empty working copy commit. When updating to another rev (eg the base commit), that commit (and change) is thrown out. When it wants to restore back to it, it no longer exists. Sadness ensues.

This is where it would be nice if we used JJ for everything: mach try perf could go wild making commits and jumping around the graph, then we could rollback to the state before anything happened just by recording the initial operation id. Maybe someday.

Ok, one option is to change our friend head_ref to not use @ (the working copy change) unless it's nonempty. That follows the pattern used in other places in jj.py -- with jj, the working copy is considered to be "just another commit" and so can be pushed to try or whatever without needing to explicitly turn it into a commit (it already is), but often it'll be empty and uninteresting so its parent is used instead.

Hm... actually, that should Just Work. I may want to do a skim of other uses of vcs.head_ref just in case...

• gecko_taskgraph/main.py... the change will fix the same bug
• tryselect/selectors/compare.py is this one
• tests: require fixup. One cosmetic, one behavior change, but the latter makes it look more similar to the other VCSes so it's all good.

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1978945 - do not use a discardable vcs.head_ref to avoid failure when updating back to it. r=ahochheiden

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/792750b0360b
https://hg.mozilla.org/integration/autoland/rev/de92de89e3b8
do not use a discardable vcs.head_ref to avoid failure when updating back to it. r=ahochheiden

Comment 9

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/de92de89e3b8