Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add parse and compare hooks #1291

Closed

Conversation

olmokramer
Copy link
Contributor

@olmokramer olmokramer commented Sep 8, 2023

This adds 4 hooks that can be set on the JPlagOptions class:

  • A pre-parsing stage hook, which is called before all submissions are parsed. It is called with the list with all submission paths eligible for the comparison, i.e. all paths that are not filtered out by SubmissionSetBuilder.isExcludedEntry.
  • A post-parsing hook per submission, which is called after the submission has been parsed. It is called with the Submission object that has been parsed.
  • A pre-comparing stage hook, which is called before the comparisons are started. It is called with a list of all comparison tuples.
  • A post-comparing hook, which is called after each comparison. It is called with the SubmissionTuple that has just been compared.

The default for each hook is to do nothing.

For some background, we are using JPlag for plagiarism checking and want to be able to track the progress of the plagiarism check (e.g. the percentage of submissions that have already been parsed, or the amount of pairs that have already been compared). With these hooks we can implement that, but simultaneously they might be used for other unforeseen use cases.

Looking forward to feedback on the concept and the implementation.

@olmokramer olmokramer changed the title Bump tough-cookie from 4.1.2 to 4.1.3 in /report-viewer Add parse and compare hooks Sep 8, 2023
@olmokramer olmokramer force-pushed the feature/add-parse-and-compare-hooks branch from 48ecef4 to b72c61f Compare September 8, 2023 14:24
@sebinside sebinside requested a review from tsaglam September 8, 2023 15:16
@sebinside sebinside added the enhancement Issue/PR that involves features, improvements and other changes label Sep 8, 2023
@olmokramer olmokramer force-pushed the feature/add-parse-and-compare-hooks branch from b72c61f to 38babe6 Compare September 12, 2023 11:36
Copy link
Member

@dfuchss dfuchss left a comment

Choose a reason for hiding this comment

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

I've made a short code review and supplied some suggestions, feel free to comment on them :)

core/src/main/java/de/jplag/options/JPlagOptions.java Outdated Show resolved Hide resolved
core/src/main/java/de/jplag/options/JPlagOptions.java Outdated Show resolved Hide resolved
core/src/main/java/de/jplag/options/JPlagOptions.java Outdated Show resolved Hide resolved
@olmokramer olmokramer force-pushed the feature/add-parse-and-compare-hooks branch 3 times, most recently from d838320 to 40364e7 Compare September 13, 2023 15:36
@olmokramer
Copy link
Contributor Author

@dfuchss Thank you for the review! I've applied most of your suggestions and replied to your comments if I did not.

@dfuchss
Copy link
Member

dfuchss commented Sep 14, 2023

@olmokramer thanks a lot. Could you please resolve the code smells as well :)

EDIT: I've retriggered SonarCloud .. let's wait for the results :)

@olmokramer
Copy link
Contributor Author

@dfuchss thanks. I've now added some test cases for all the hooks as well.

Copy link
Member

@dfuchss dfuchss left a comment

Choose a reason for hiding this comment

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

Thanks for the tests. I've one minor suggestion regarding them.

core/src/test/java/de/jplag/HooksTest.java Outdated Show resolved Hide resolved
@olmokramer olmokramer force-pushed the feature/add-parse-and-compare-hooks branch from 06c8f73 to 1ed2cc6 Compare September 15, 2023 13:48
Copy link
Member

@dfuchss dfuchss left a comment

Choose a reason for hiding this comment

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

I think the idea and realization is good.
Maybe you can add some docs to the public methods/fields you've created.

Afterwards, let's wait for the review of @tsaglam :)

@dfuchss
Copy link
Member

dfuchss commented Sep 15, 2023

Some tests seem to fail :)

@olmokramer olmokramer force-pushed the feature/add-parse-and-compare-hooks branch from 1ed2cc6 to 0be6f36 Compare September 18, 2023 08:26
@olmokramer
Copy link
Contributor Author

@dfuchss Thanks again for the reviews. I have added documentation for the new fields on the JPlagOptions class.

Some tests seem to fail :)

Whoops, shouldn't have copy-pasted that test... Fixed now :)

@olmokramer olmokramer force-pushed the feature/add-parse-and-compare-hooks branch from 0be6f36 to c8b2379 Compare September 18, 2023 14:39
@tsaglam
Copy link
Member

tsaglam commented Sep 25, 2023

Sorry for my absence, I was out of office. I will take a look at it as soon as I can find the time!

@tsaglam tsaglam added the minor Minor issue/feature/contribution/change label Sep 26, 2023
This adds 4 hooks that can be set on the `JPlagOptions` class:

- A pre-parsing stage hook, which is called before all submissions are
  parsed. It is called with the list with all submission paths eligible
  for the comparison, i.e. all paths that are not filtered out by
  `SubmissionSetBuilder.isExcludedEntry`.
- A post-parsing hook per submission, which is called after the
  submission has been parsed. It is called with the `Submission`
  object that has been parsed.
- A pre-comparing stage hook, which is called before the comparisons
  are started. It is called with a list of all comparison tuples.
- A post-comparing hook, which is called after each comparison. It
  is called with the `SubmissionTuple` that has just been compared.
@olmokramer olmokramer force-pushed the feature/add-parse-and-compare-hooks branch from c8b2379 to 55624bd Compare October 3, 2023 09:08
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 3, 2023

[JPlag Plagiarism Detector] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@olmokramer
Copy link
Contributor Author

@tsaglam Have you already had a chance to take a look at this?

@tsaglam
Copy link
Member

tsaglam commented Nov 7, 2023

Have you already had a chance to take a look at this?

Sorry, I am currently swamped and probably will be until the end of November.
For now, I will have @TwoOfTwelve take a look to keep the ball rolling.

@tsaglam tsaglam requested a review from TwoOfTwelve November 7, 2023 15:46
@olmokramer
Copy link
Contributor Author

Thank you for your response and good luck with your work the upcoming month :-)

@TwoOfTwelve TwoOfTwelve self-assigned this Nov 13, 2023
@TwoOfTwelve
Copy link
Contributor

TwoOfTwelve commented Nov 18, 2023

@olmokramer
I have talked with Timur about your code. There are a couple of thinks we agreed are not good for the architecture of JPlag. The major points are:

  1. The JPlagOptions class is expanded further, but is already very big and clunky.
  2. JPlagOptions has a dependency on Submission and some other types, which should not happen
  3. Hooks are not really options for JPlag
  4. The JPlag code get coupled tightly with the hooks
  5. There is a central list of available hooks, which would make it hard to expand them in the future

I have implemented a suggestion on how we could handle the hooks in the feature/hooks branch. Please take a look on that an tell me what you think. In particular if it fulfills all your requirements. Right now it is still in a rough state and I only implemented one hook as an example. If you like the idea, I would leave it up to you how we proceed. If you want you can change your implementation (or make a new one), or I could finish my changes on feature/hooks.

I am sorry for any inconvenience this might cause you. We do appreciate your work, we are just trying to keep our architecture as clean as possible.

@olmokramer
Copy link
Contributor Author

@TwoOfTwelve thank you for the review! This is exactly the kind of feedback I was looking for :) I agree with the points you brought up and that the current implementation is not necessarily the best for the JPlag architecture.

I'm also on a quite tight schedule, so I don't know when I will have time to take a look at your suggested implementation. We are running a build of our fork with this PR, so there's no real rush from our side to get this in. I'd rather get it done in a way that is maintainable and sustainable. I'll try to make some time next week, though, to take a look.

Our requirements are not very complex. In particular, we need to be able to

  • get the total amount of submissions that will be parsed, before parsing has started. Though it would also be fine to get this information as soon as the first submission has been parsed. (This is what the preParseHook in this PR is for).

  • update a counter as soon as a submission has been parsed. (This is what the parseHook in this PR is used for)

  • get the total amount of comparisons that will be made, before comparing has started. Though here too it would be fine to get this information directly after the first comparison. (This is what we use the preCompareHook from this PR for)

  • update a counter as soon as a comparison between 2 submissions has finished. (We use the compareHook from this PR for that)

Thanks again for you consideration.

@tsaglam
Copy link
Member

tsaglam commented Feb 16, 2024

Superseded by #1433. Let us know if additional requirements come up.

@tsaglam tsaglam closed this Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issue/PR that involves features, improvements and other changes minor Minor issue/feature/contribution/change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants