Skip to content

Add chapter-level data counts to execution data#990

Open
Enkidu93 wants to merge 3 commits into
mainfrom
chapter-level-data-counts
Open

Add chapter-level data counts to execution data#990
Enkidu93 wants to merge 3 commits into
mainfrom
chapter-level-data-counts

Conversation

@Enkidu93

@Enkidu93 Enkidu93 commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Fixes #907

@ddaspit, my only outstanding question is how we'd like to count only pretranslated non-verse text. At the moment, they're all lumped together. We could either not count them at all or count them separately or just lump them in (current behavior). What do you think makes the most sense?


This change is Reviewable

@Enkidu93
Enkidu93 requested a review from ddaspit July 1, 2026 17:25
@codecov-commenter

codecov-commenter commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 72.50000% with 88 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.87%. Comparing base (12be51e) to head (3339e67).

Files with missing lines Patch % Lines
...Shared/Services/WordAlignmentPreprocessBuildJob.cs 57.01% 44 Missing and 5 partials ⚠️
...e.Shared/Services/TranslationPreprocessBuildJob.cs 80.17% 16 Missing and 7 partials ⚠️
...l.Machine.Shared/Services/NmtPreprocessBuildJob.cs 70.58% 4 Missing and 1 partial ⚠️
...hared/Services/ServalTranslationPlatformService.cs 0.00% 5 Missing ⚠️
...red/Services/ServalWordAlignmentPlatformService.cs 0.00% 5 Missing ⚠️
...rval.Machine.Shared/Services/PreprocessBuildJob.cs 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #990      +/-   ##
==========================================
+ Coverage   65.54%   65.87%   +0.33%     
==========================================
  Files         387      388       +1     
  Lines       21747    21952     +205     
  Branches     2770     2782      +12     
==========================================
+ Hits        14253    14460     +207     
+ Misses       6454     6440      -14     
- Partials     1040     1052      +12     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Enkidu93

Enkidu93 commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator Author

I guess I should add this to Echo as well. I'll add that.

@Enkidu93

Enkidu93 commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator Author

I guess I should add this to Echo as well. I'll add that.

I'm realizing that I'll need a Machine dependency in Echo in order to do that. Is that OK or would you prefer to just wait until we complete #987 and let it be resolved there?

@Enkidu93
Enkidu93 force-pushed the chapter-level-data-counts branch from 2363b49 to e52b0c5 Compare July 2, 2026 16:30
@Enkidu93

Enkidu93 commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

Our conclusion was that we will only count scripture segments in this new counts data structure.

@ddaspit ddaspit left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ddaspit reviewed 20 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Enkidu93).


src/Machine/src/Serval.Machine.Shared/Models/BuildExecutionData.cs line 9 at r1 (raw file):

    public bool? IsTrainFilteredByChapter { get; init; }
    public bool? IsInferenceFilteredByChapter { get; init; }
    public Dictionary<string, Dictionary<string, int>>? TrainVerseCount { get; init; }

Can we use IReadOnlyDictionary? Does it cause issues?

@Enkidu93 Enkidu93 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@Enkidu93 made 1 comment.
Reviewable status: 7 of 21 files reviewed, 1 unresolved discussion (waiting on ddaspit).


src/Machine/src/Serval.Machine.Shared/Models/BuildExecutionData.cs line 9 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Can we use IReadOnlyDictionary? Does it cause issues?

Done. No, I double-checked and I think we're fine to use it. It makes passing it through all the layers a little uglier is all - esp. since it is nested.

@Enkidu93

Enkidu93 commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

I guess I should add this to Echo as well. I'll add that.

I'm realizing that I'll need a Machine dependency in Echo in order to do that. Is that OK or would you prefer to just wait until we complete #987 and let it be resolved there?

I decided to leave on Echo for now. I can add support if you prefer but hopefully we will do that work in this sprint as well anyways.

@Enkidu93

Enkidu93 commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Let me know what you think of my refactoring to be able to better test the preprocess row processing.

@Enkidu93
Enkidu93 requested a review from ddaspit July 14, 2026 19:45

@ddaspit ddaspit left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ddaspit reviewed all commit messages and made 1 comment.
Reviewable status: 7 of 21 files reviewed, 1 unresolved discussion (waiting on Enkidu93).


src/Machine/src/Serval.Machine.Shared/Models/BuildExecutionData.cs line 9 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Done. No, I double-checked and I think we're fine to use it. It makes passing it through all the layers a little uglier is all - esp. since it is nested.

The nested dictionary could be Dictionary if that makes it easier.

@Enkidu93
Enkidu93 force-pushed the chapter-level-data-counts branch from c96729a to 3339e67 Compare July 17, 2026 17:09

@Enkidu93 Enkidu93 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@Enkidu93 made 1 comment.
Reviewable status: 7 of 21 files reviewed, 1 unresolved discussion (waiting on ddaspit).


src/Machine/src/Serval.Machine.Shared/Models/BuildExecutionData.cs line 9 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

The nested dictionary could be Dictionary if that makes it easier.

It's fine 🤷. It's up to you - I can switch to just a Dictionary if you prefer to avoid the re-dictionary-ing.

@ddaspit ddaspit left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ddaspit made 1 comment.
Reviewable status: 7 of 21 files reviewed, 1 unresolved discussion (waiting on Enkidu93).


src/Machine/src/Serval.Machine.Shared/Models/BuildExecutionData.cs line 9 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

It's fine 🤷. It's up to you - I can switch to just a Dictionary if you prefer to avoid the re-dictionary-ing.

I've used a concrete class for a nested collection in the past, because it was ugly. Let's go ahead and do it.

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.

Partial book drafting chart

3 participants