Skip to content

ATM: Remove redundant code - #11321

Merged
tiferet merged 6 commits into
mainfrom
tiferet/complexity-reduction
Nov 29, 2022
Merged

ATM: Remove redundant code#11321
tiferet merged 6 commits into
mainfrom
tiferet/complexity-reduction

Conversation

@tiferet

@tiferettiferet commented Nov 17, 2022

Copy link
Copy Markdown
Contributor

This PR is the code cleanup made possible by the previous few PRs. As such, it has a lot of lines of change (mostly code deletion 🥳), but the changes are conceptually small. I tried to make the commits as clear as possible, tackling one piece of complexity reduction per commit, and explaining it in the commit comment. Commit-by-commit review warmly recommended 😄

Note that this PR is based on the branch tiferet/endpoint-filters, because that branch has not yet been merged and I don't want to see the commits from #11281 in this PR as well.

main code deletions / simplifications

  • isOtherModeledArgument and isArgumentToBuiltinFunction contained the old logic for selecting negative endpoints for training. These can now be deleted, and replaced by a single base class that collects all EndpointCharacteristics that are currently used to indicate negative training samples: OtherModeledArgumentCharacteristic. This in turn lets us delete code from StandardEndpointFilters that effectively said that endpoints that are high-confidence non-sinks shouldn't be scored at inference time, either.
  • FilteringReason is no longer being used and can be deleted.
  • Delete CoreKnowledge and StandardEndpointFilters: All remaining functionality in CoreKnowledge and StandardEndpointFilters is only being used in EndpointCharacteristics, so it can be moved there as a small set of helper predicates.

Timing checks

Closeshttps://github.com/github/ml-ql-adaptive-threat-modeling/issues/2110

@tiferet
tiferet marked this pull request as ready for review November 18, 2022 00:36
@tiferet
tiferet requested review from a team and kaeluka and removed request for a teamNovember 18, 2022 00:36
@tiferet
tiferetforce-pushed the tiferet/complexity-reduction branch 2 times, most recently from 9a37061 to fac6641CompareNovember 21, 2022 16:34

@kaelukakaeluka left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approve

This is the kind of PR where I'm extra glad we have CI checks: big surface, but not much new logic ;)

I'll give this a formal approval once the first PR is merged and this has been rebased; but I've done the review now and it all looks good to me. I've left one question for my own understanding.

@owen-mcowen-mc changed the title Remove redundant codeATM: Remove redundant codeNov 22, 2022
`isOtherModeledArgument` and `isArgumentToBuiltinFunction` contained the old logic for selecting negative endpoints for training.
These can now be deleted, and replaced by a single base class that collects all EndpointCharacteristics that are currently used to indicate negative training samples: `OtherModeledArgumentCharacteristic`.
This in turn lets us delete code from `StandardEndpointFilters` that effectively said that endpoints that are high-confidence non-sinks shouldn't be scored at inference time, either.
All remaining functionality in `CoreKnowledge` is only being used in `EndpointCharacteristics`, so it can be moved there as a small set of helper predicates.
All remaining functionality in `StandardEndpointFilters` is only being used in `EndpointCharacteristics`, so it can be moved there as a small set of helper predicates.
@tiferet
tiferetforce-pushed the tiferet/complexity-reduction branch from fac6641 to 4580b55CompareNovember 28, 2022 19:36
@tiferet

Copy link
Copy Markdown
ContributorAuthor

✅ Approve

This is the kind of PR where I'm extra glad we have CI checks: big surface, but not much new logic ;)

I'll give this a formal approval once the first PR is merged and this has been rebased; but I've done the review now and it all looks good to me. I've left one question for my own understanding.

I rebased this on top of the latest version of tiferet/endpoint-filters, so if #11281 requires no more changes you can now go ahead and approve this one too. Thank you!

kaeluka
kaeluka previously approved these changes Nov 29, 2022
Base automatically changed from tiferet/endpoint-filters to mainNovember 29, 2022 20:38
@tiferet

Copy link
Copy Markdown
ContributorAuthor

@kaeluka to merge this into main I had to remove a merge conflict due to a tiny comment change in CoreKnowledge.qll which gets deleted in this PR. So now I need you to re-approve this before I can merge it 😞

@tiferet

Copy link
Copy Markdown
ContributorAuthor

@kaeluka to merge this into main I had to remove a merge conflict due to a tiny comment change in CoreKnowledge.qll which gets deleted in this PR. So now I need you to re-approve this before I can merge it 😞

@adityasharad Thank you! ❤️

@kaeluka Never mind! 😄

@tiferet
tiferet merged commit 2241252 into mainNov 29, 2022
@tiferet
tiferet deleted the tiferet/complexity-reduction branch November 29, 2022 21:17
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@tiferet@kaeluka@adityasharad