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

Concurrent Enrichment Extractions #78

Merged
merged 43 commits into from
Aug 8, 2024
Merged

Conversation

danielaboost
Copy link
Contributor

@danielaboost danielaboost commented Aug 1, 2024

Acceptance Criteria

  • Enrichments can be configure to be run concurrently
  • Any other improvement to enrichments are welcome
  • consider what settings might be useful to customise / safeguard this process

Notes

  • If you look at the Act, Bills, Regulation pipelines enrichments, they are a good example of enrichments we would like to speed up
  • not for every use case

What we've done

  • Created a new worker to run the job async when the setting is selected for running the pipeline. The existing logic has been moved into a concern so the same logic can be used both for async runs and non-async runs

Screenshot 2024-08-07 at 11 07 40 AM

Copy link

github-actions bot commented Aug 1, 2024

Code quality score

Nice work!! The code quality has improved for this PR! ✨ 🌈 🎉 🌟

Ruby file count Similarity score (flay) ABC complexity (flog) Code smells (reek) TOTALS
base 90 6.12 5.36 17.2 28.68
this branch 94 6.17 5.28 17.2 28.65
difference 4 ⚠️ 0.05 -0.08 0.0 -0.03

Daniela Lemow added 28 commits August 1, 2024 14:03
@danielaboost
Copy link
Contributor Author

Code quality score

Uh oh! The code quality got worse for this PR! Better take a look!! 🚨

Ruby file count Similarity score (flay) ABC complexity (flog) Code smells (reek) TOTALS
base 90 6.12 5.36 17.2 28.68
this branch 93 6.17 5.36 17.25 28.78
difference 3 ⚠️ 0.05 0.0 ⚠️ 0.05 0.1

I have refactored this A LOT. At the beginning the CQ increase was a lot worse, and i've managed to get it to here which is far better. The remaining reeks are things that I don't necessarily agree with, such as moving the EnrichmentExtractionProcess concern private methods into a utility class (which I don't love as I want to keep the logic near where its being used since they should only ever be used within the process_enrichment_extraction method).

Needless to say I am comfortable with the increase and have thought about this a lot 😁

Copy link
Contributor

@richardmatthewsdev richardmatthewsdev left a comment

Choose a reason for hiding this comment

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

Thanks @danielaboost! Very cool

Copy link

@tom-mcculloch tom-mcculloch left a comment

Choose a reason for hiding this comment

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

Nice work mate, cool improvement!

@danielaboost danielaboost merged commit e40367d into main Aug 8, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants