Investigate atime CI failure for setDT update

[MichaelChirico]

          @Anirban166 any idea what's going on with the {atime} CI failure?

Error in value[[3L]](cond) : 
  Error in revparse_single(object, branch): Error in 'git2r_revparse_single': Requested object could not be found

 when trying to checkout 1872f473b20fdcddc5c1b35d79fe9229cd9a1d15

Originally posted by @MichaelChirico in #6106 (comment)

[MichaelChirico]

@Anirban166

---

@Anirban166 any idea what's going on with the {atime} CI failure?

Error in value[[3L]](cond) : 
  Error in revparse_single(object, branch): Error in 'git2r_revparse_single': Requested object could not be found

 when trying to checkout 1872f473b20fdcddc5c1b35d79fe9229cd9a1d15

It appears that it is trying to install a data.table version that doesn't belong to any branch here: 1872f47

This was previously working for us since the setdt-wide branch existed, but now that it has been deleted, none of the commits within that PR would be valid since they are not a part of the current data.table repository (no branch, no references)

Also @tdhock I think this is why the last commits in some PRs don't work (because their branches have been deleted) but the merge commits work (because they are integrated or stay as a part of data.table)

[MichaelChirico]

@Anirban166

---

@MichaelChirico I believe the easiest fix here would be to hit 'Restore branch' for setdt-wide towards the end of #5427

(I've encountered this issue before and in that case too, the main reason had to do with certain branches being non-existent as I did a shallow fork of data.table and did not retrieve the entire set of branches and commits)

[MichaelChirico]

Hmm, I don't have any trouble running git checkout 1872f473b20fdcddc5c1b35d79fe9229cd9a1d15 locally though...

[MichaelChirico]

I see. Somehow my local repo still has that commit available, even though the branch is removed. Some git voodoo. If I re-clone data.table to the /tmp, I can't check it out:

git checkout 1872f473b20fdcddc5c1b35d79fe9229cd9a1d15
# fatal: reference is not a tree: 1872f473b20fdcddc5c1b35d79fe9229cd9a1d15

OK, so we need to keep the merged branches around whenever they're part of a regression suite, is that right?

[MichaelChirico]

Actually, the following works (?):

git fetch origin  1872f473b20fdcddc5c1b35d79fe9229cd9a1d15
git checkout 1872f473b20fdcddc5c1b35d79fe9229cd9a1d15

So we could instead have a step in the CI to fetch all of the needed commits?

[Anirban166]

OK, so we need to keep the merged branches around whenever they're part of a regression suite, is that right?

Not necessarily. It might be good to have all of those branches for consistency, but if it's deleted/removed, then too it's fine as we can just use a commit from the master branch following such PRs for instance (if not the merge commit itself).

Also @tdhock I think this is why the last commits in some PRs don't work (because their branches have been deleted) but the merge commits work (because they are integrated or stay as a part of data.table)

I believe the easiest fix here would be to hit 'Restore branch' for setdt-wide towards the end of #5427

Like I mentioned above, this was just the easiest/fastest route (as I saw) to fix it, as otherwise I would just use the merge commit which is always available since it's in the git history of master (even when one does a fork of data.table on GitHub they will always have the master branch)

I tested it a while ago after syncing my fork, so if this sounds good to you I can go for a PR to make the change.

So we could instead have a step in the CI to fetch all of the needed commits?

This could work too, but I'm not sure if it would be good practice to keep adding commits in this way for each test case..

[MichaelChirico]

This could work too, but I'm not sure if it would be good practice to keep adding commits in this way for each test case..

Right, I definitely wouldn't want to add a separate commit each time to record these incrementally. Instead we would want to think of a way for the CI to populate these automatically, e.g.

refs = '.ci/atime/tests.R' |>
  parse(keep.source=TRUE) |>
  getParseData() |>
  subset(token == "STR_CONST", "text", drop=TRUE) |>
  vapply(str2lang, "", USE.NAMES=FALSE) |>
  grep(x=_, "^[a-z0-9]{40}$", value=TRUE)
system2("git", c("fetch", "origin", refs))

That will fetch every 40-character hash found in tests.R.

[tdhock]

Thanks for the analysis Michael. This is a known issue and the current solution is just click "restore branch" so can we do that? and/or avoid deleting branches from PRs where we have commits that we want to test?

helpful to know about fetch origin trick

[MichaelChirico]

I guess the question is how many such branches we think we'll have around in say 5 years -- O(10) or O(100)? In the latter case I'd rather do the git fetch approach so as not to clutter the repo with dead+ancient branches just for benchmark testing.

If the former, yes it's fine to just keep those branches around.

[tdhock]

100 performance tests would be great but I think a 10-20 is more likely
we can always implement the fetch approach later if necessary

[MichaelChirico]

OK, just to check, @Anirban166 do you have permissions to restore branches?

[Anirban166]

OK, just to check, @Anirban166 do you have permissions to restore branches?

Yup

[Anirban166]

@MichaelChirico I think we can close this since the problem as per the title of this issue has been solved, noh? (saying based on this)
We can reopen it when you think would be appropriate to switch to the git fetch approach later on.