Java: Automodel: Add Candidates for Regression Testing

[kaeluka]

This adds additional candidates that are useful for regression testing.

For regression testing, we want to also look at candidates that we used to extract and then modeled using automodel.

To achieve that, we add the candidates to the extraction query, but also add metadata so that we can skip them during non-testing uses.

[tausbn]

Short and sweet! 👍

I have a small suggestion for how to make this more flexible (in case we need it), but otherwise this looks good to me.

[jhelie]

⚠️ This is based on another PR (#13886) that is currently under review and should be merged first. It is sufficient to review starting with commit 551b34e

drive-by comment: I do that quite often too and I find that selecting the previous PR's branch as the merge target for the second PR is quite handy to keep things clear.

[jhelie]

Thanks @kaeluka! Just to double check my understanding: this design does mean that during normal operation (i.e. not regression testing) we need to implement a filter in the script that will skip the candidates with an ai-{foo} provenance, correct?

In practice I imagine that the majority of candidates will not have an AI-provenance and so that skipping will be the exception (and so won't impact performance) but ooi have we also considered a 2 queries design whereby we would add a AutomodelApplicationModeExtractCandidatesRegression.ql query?

[kaeluka]

Just to double check my understanding: this design does mean that during normal operation (i.e. not regression testing) we need to implement a filter in the automodel.py script that will skip the candidates with an ai-{foo} provenance, correct?

Yes! 👍

In practice I imagine that the majority of candidates will not have an AI-provenance and so that skipping will be the exception (and so won't impact performance) but ooi have we also considered a 2 queries design whereby we would add a AutomodelApplicationModeExtractCandidatesRegression.ql query?

I have, and have discussed that in our standup on Monday, remember? Two queries will mean increased maintenance load, even though the two queries would be almost exactly the same.

[kaeluka]

drive-by comment: I do that quite often too and I find that selecting the previous PR's branch as the merge target for the second PR is quite handy to keep things clear.

Thanks! I considered that but have never done it before because I wasn't sure how the UI would present that! Will do that next time ;)

Edit: now that the 'base' branch was merged, I could update this PR and it only shows the new commits as intended.

[jhelie]

Shall we have similar changes for framework mode in the same PR?

[jhelie]

Two queries will mean increased maintenance load, even though the two queries would be almost exactly the same.

It feels a little weird to always extract all candidates instead of just the ones we need (seems like the wrong away around) but we can easily modify that later if need be.

In the meantime we should probably avoiding merging this until we have the filter implemented in the script.

[jhelie]

I've just realised that the positive and negative extraction queries do not include the new alreadyAiModeled field. I'll try to add it in since t's more robust to have the same extracted features for examples and candidates (this is what allow us to easily classify candidates, regardless of whether or not they are known models, which can be useful for some regression testing).

edit: come to think of it I'm not entirely of the strength of the use case - we can handle the diff script side easily.

[jhelie]

In the meantime we should probably avoiding merging this until we have the filter implemented in the script.

This is actually not true - it will only be an issue when we attempt bumping our codeql dependency.

[jhelie]

Shall we have similar changes for framework mode in the same PR?

@tausbn thanks for the pointers on how to update the tests, I think the commit I just pushed took care of this - if you're happy with it I think this PR is now ready to be merged.

[tausbn]

As far as I can tell, all of your test cases have alreadyAiModeled set to false (as witnessed by the file://:1:1:1:1 bit that precedes them -- i.e. the empty string).

This seems... not right? I would expect at least one of the test cases to actually populate this with a non-empty string (like we do for the application mode tests, where in one case it's "ai-manual").

[jhelie]

No there is one that has the ai-manual provenance: de76c07 (so that's indeed the same situation as for the application mode)

See the 8th example in AutomodelFrameworkModeExtractCandidates.expected.

[tausbn]

No there is one that has the ai-manual provenance: de76c07 (so that's indeed the same situation as for the application mode)

See the 8th example in AutomodelFrameworkModeExtractCandidates.expected.

Ah, so there is! Never mind, then. 🙂

[tausbn]

[jhelie]

Great thanks for having a quick look, let's merge it then! 🚢