Scripture Update block #168

johnml1135 · 2025-04-03T20:48:05Z

Will this evolve first?

This change is

ddaspit

This looks good. I just have a couple of comments.

Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @johnml1135)

machine/corpora/scripture_update_block.py line 11 at r1 (raw file):

    def __init__(self) -> None:
        self._ref: ScriptureRef = ScriptureRef()

What is this field used for?

machine/corpora/scripture_update_block.py line 12 at r1 (raw file):

    def __init__(self) -> None:
        self._ref: ScriptureRef = ScriptureRef()
        self._elements: list[ScriptureUpdateElement] = []

If this is accessible outside of this class, then I would expose it as a public read-only property.

@property
def elements(self) -> List[ScriptureUpdateElement]:
    return self._elements

machine/corpora/scripture_update_block_handler_base.py line 6 at r1 (raw file):

class ScriptureUpdateBlockHandlerBase:

This can just be an interface. You can rename it to ScriptureUpdateBlockHandler.

machine/corpora/usfm_parser_state.py line 112 at r1 (raw file):

    def is_verse_text(self) -> bool:
        # anything before verse 1 is not verse text
        if self.verse_ref.verse_num == 0:

I am a little worried about this change. In this context, is_verse_text is referring to the UsfmTextType. Just to be safe, I think we should remove this change and add the check to UpdateUsfmParserHandler.end_para().

machine/corpora/scripture_update_element.py line 16 at r1 (raw file):

@dataclass
class ScriptureUpdateElement:

I think ScriptureUpdateOperation would be a more precise name, unless I'm misunderstanding the purpose of this class. "Element" is kind of generic.

johnml1135 · 2025-04-10T17:01:06Z

machine/corpora/scripture_update_block.py line 11 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

What is this field used for?

Some functionality will have to be aware of the scripture passage to know how to process the verse text. For instance, if Ben's algorithm fails in a chapter, that entire chapter should be left alone - this is facilitated by being verse-aware.

johnml1135 · 2025-04-10T17:18:09Z

machine/corpora/scripture_update_block.py line 12 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

If this is accessible outside of this class, then I would expose it as a public read-only property.
@property
def elements(self) -> List[ScriptureUpdateElement]:
    return self._elements

Done.

johnml1135 · 2025-04-10T17:20:43Z

machine/corpora/scripture_update_block_handler_base.py line 6 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

This can just be an interface. You can rename it to ScriptureUpdateBlockHandler.

Done.

johnml1135 · 2025-04-10T17:21:39Z

machine/corpora/scripture_update_element.py line 16 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I think ScriptureUpdateOperation would be a more precise name, unless I'm misunderstanding the purpose of this class. "Element" is kind of generic.

These are the elements that are updated - they are not the operations themselves. I don't know if there is a better way to represent that.

johnml1135 · 2025-04-10T17:22:55Z

machine/corpora/usfm_parser_state.py line 112 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I am a little worried about this change. In this context, is_verse_text is referring to the UsfmTextType. Just to be safe, I think we should remove this change and add the check to UpdateUsfmParserHandler.end_para().

Let's discuss the implications for the different scenarios.

ddaspit

Reviewed 13 of 13 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @johnml1135)

machine/corpora/scripture_update_block.py line 11 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

Some functionality will have to be aware of the scripture passage to know how to process the verse text. For instance, if Ben's algorithm fails in a chapter, that entire chapter should be left alone - this is facilitated by being verse-aware.

Ok, then you should expose it as a public property.

machine/corpora/scripture_update_element.py line 16 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

These are the elements that are updated - they are not the operations themselves. I don't know if there is a better way to represent that.

Ok, I think I understand better what the class represents.

tests/corpora/test_usfm_file_text.py line 69 at r2 (raw file):

    rows = list(text)

    assert len(rows) == 52

Why did this unit test change? It worries me that changing the USFM updater is affecting USFM parsing.

machine/corpora/scripture_update_block_handler.py line 10 at r2 (raw file):

class ScriptureUpdateBlockHandler(ABC):

    def process_block(self, block: ScriptureUpdateBlock) -> ScriptureUpdateBlock:

This method should not have an implementation, like other interfaces in Machine.py.

@abstractmethod
def process_block(self, block: ScriptureUpdateBlock) -> ScriptureUpdateBlock:
    ...

johnml1135 · 2025-04-11T19:24:42Z

machine/corpora/scripture_update_block.py line 11 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Ok, then you should expose it as a public property.

Yes... that makes sense now :-)

johnml1135 · 2025-04-11T20:18:39Z

machine/corpora/scripture_update_block_handler.py line 10 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

This method should not have an implementation, like other interfaces in Machine.py.
@abstractmethod
def process_block(self, block: ScriptureUpdateBlock) -> ScriptureUpdateBlock:
    ...

Paradigms...

johnml1135 · 2025-04-11T20:19:55Z

tests/corpora/test_usfm_file_text.py line 69 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Why did this unit test change? It worries me that changing the USFM updater is affecting USFM parsing.

It's related to the "is verse text" If there are two '\p' before a verse, are they separate enties to be translated differently or should the second one be a true "paragraph" while the first is a "segment initiating paragraph"?

codecov-commenter · 2025-04-14T19:18:12Z

Codecov Report

Attention: Patch coverage is 98.72774% with 5 lines in your changes missing coverage. Please review.

Project coverage is 88.66%. Comparing base (7a4bde9) to head (402a03f).

Files with missing lines	Patch %	Lines
machine/corpora/update_usfm_parser_handler.py	97.63%	3 Missing ⚠️
machine/corpora/usfm_update_block.py	97.36%	1 Missing ⚠️
...chine/corpora/zip_paratext_project_text_updater.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #168      +/-   ##
==========================================
+ Coverage   88.64%   88.66%   +0.02%     
==========================================
  Files         277      280       +3     
  Lines       16632    16708      +76     
==========================================
+ Hits        14743    14814      +71     
- Misses       1889     1894       +5

☔ View full report in Codecov by Sentry.
📢 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.

ddaspit

Reviewed 7 of 8 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @johnml1135)

machine/corpora/usfm_parser_state.py line 112 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

Let's discuss the implications for the different scenarios.

I'm good with this change, since it fixes a bug.

tests/corpora/test_usfm_file_text.py line 69 at r2 (raw file):

Previously, johnml1135 (John Lambert) wrote…

It's related to the "is verse text" If there are two '\p' before a verse, are they separate enties to be translated differently or should the second one be a true "paragraph" while the first is a "segment initiating paragraph"?

I see. I think it makes sense to output a non-verse segment for text that occurs before the first verse marker in a paragraph. The fact that it didn't output a row before was probably a bug. Can we add an assert to check one of these empty non-verse paragraph segments?

machine/corpora/paratext_project_terms_parser_base.py line 1 at r4 (raw file):

from __future__ import annotations

Why was this added?

machine/corpora/scripture_update_element.py line 14 at r4 (raw file):

    INSERTED_TEXT = auto()
    PARAGRAPH = auto()
    EMBED_BLOCK = auto()

What is the difference between an embed and an embed block?

johnml1135 · 2025-04-23T12:28:19Z

machine/corpora/paratext_project_terms_parser_base.py line 1 at r4 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Why was this added?

My mistake.

johnml1135 · 2025-04-23T12:29:04Z

machine/corpora/scripture_update_element.py line 14 at r4 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

What is the difference between an embed and an embed block?

An embed is an embed marker. An embed block is an entire block. Maybe a different term would be better, such as CHILD_BLOCK or similar.

johnml1135 · 2025-04-23T13:14:09Z

tests/corpora/test_usfm_file_text.py line 69 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I see. I think it makes sense to output a non-verse segment for text that occurs before the first verse marker in a paragraph. The fact that it didn't output a row before was probably a bug. Can we add an assert to check one of these empty non-verse paragraph segments?

It is tested in test_paragraph_in_verse - the paragraph before the verse is not stripped with "strip paragraphs".

johnml1135 · 2025-04-23T13:14:46Z

machine/corpora/paratext_project_terms_parser_base.py line 1 at r4 (raw file):

Previously, johnml1135 (John Lambert) wrote…

My mistake.

Actually it was added because of the updated linting changes with [ElementTree.Element]. It doesn't work with older versions of python.

ddaspit

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @johnml1135)

machine/corpora/paratext_project_terms_parser_base.py line 1 at r4 (raw file):

Previously, johnml1135 (John Lambert) wrote…

Actually it was added because of the updated linting changes with [ElementTree.Element]. It doesn't work with older versions of python.

I don't receive a linting error on 3.9, which is the minimum version that we support.

machine/corpora/scripture_update_element.py line 14 at r4 (raw file):

Previously, johnml1135 (John Lambert) wrote…

An embed is an embed marker. An embed block is an entire block. Maybe a different term would be better, such as CHILD_BLOCK or similar.

I'm still not sure what you mean by embed block. What do you mean by "entire block"?

isaac091

When a verse has one or more section headers in it and is being updated, block.ref is that of the last header when process_block is called. Minimal reproduction:
rows=[(scr_ref("MAT 1:1"), "updated verse text")]

\id MAT
\c 1
\v 1 verse text
\s1 header

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @johnml1135)

…translations. These updates make it happen.

Pass marker type (embed, style) to update block

Enkidu93

Looking at the code coverage report, it seems at least comparable to what it was before. However, there are some bits of the logic that aren't touched by the tests - maybe that's something we could revisit at some point.

Thank you for working this out to completion!

Reviewed 2 of 10 files at r1, 3 of 13 files at r2, 2 of 8 files at r3, 15 of 15 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ddaspit and @johnml1135)

machine/corpora/paratext_project_terms_parser_base.py line 1 at r4 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I don't receive a linting error on 3.9, which is the minimum version that we support.

Is this still supposed to be removed?

machine/corpora/update_usfm_parser_handler.py line 101 at r6 (raw file):

            and self._paragraph_behavior == UpdateUsfmMarkerBehavior.STRIP
        ):
            self._skip_updateable_tokens(state)

I think 'updatable' might be more standard - plus it's shorter. But I'm fine with 'updateable' too.

machine/corpora/scripture_ref_usfm_parser_handler.py line 137 at r6 (raw file):

            self._end_embed_text_wrapper(state)

    def start_note(self, state, marker, caller, category) -> None:

Parameter type hinting?

machine/corpora/scripture_ref_usfm_parser_handler.py line 140 at r6 (raw file):

        self._start_embed_text_wrapper(state, marker)

    def end_note(self, state, marker, closed) -> None:

Parameter type hinting?

ddaspit

Reviewed 15 of 15 files at r5, 3 of 3 files at r6.
Reviewable status: 24 of 25 files reviewed, 2 unresolved discussions (waiting on @Enkidu93)

machine/corpora/paratext_project_terms_parser_base.py line 1 at r4 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Is this still supposed to be removed?

Done

machine/corpora/scripture_ref_usfm_parser_handler.py line 137 at r6 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Parameter type hinting?

Done

machine/corpora/scripture_ref_usfm_parser_handler.py line 140 at r6 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Parameter type hinting?

Done

machine/corpora/update_usfm_parser_handler.py line 101 at r6 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

I think 'updatable' might be more standard - plus it's shorter. But I'm fine with 'updateable' too.

Done

ddaspit

Reviewed 6 of 6 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Enkidu93)

machine/corpora/paratext_project_terms_parser_base.py line 1 at r4 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Done

This is necessary to allow some Python versions to work with the generic ElementTree type. The generic type is needed to satisfy Pyright.

Enkidu93

Reviewed 6 of 6 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @johnml1135)

machine/corpora/paratext_project_terms_parser_base.py line 1 at r4 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

This is necessary to allow some Python versions to work with the generic ElementTree type. The generic type is needed to satisfy Pyright.

OK

ddaspit

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @johnml1135)

- Add update block handlers to `UpdateUsfmParserHandler` - no longer extract note text as a separate row in `UsfmTextBase` - no longer update note text in `UpdateUsfmParserHandler` - properly handle empty paragraphs before a verse - update `ScriptureRefUsfmParserHandler` to recognize embed text --------- Co-authored-by: Damien Daspit <[email protected]>

ddaspit requested changes Apr 4, 2025

View reviewed changes

johnml1135 mentioned this pull request Apr 8, 2025

UpdateBlock - add to Machine sillsdev/serval#667

Closed

ddaspit reviewed Apr 11, 2025

View reviewed changes

ddaspit reviewed Apr 22, 2025

View reviewed changes

ddaspit mentioned this pull request Apr 22, 2025

Translate: \s and \p markers reversed when section heading occurs mid-verse sillsdev/silnlp#708

Closed

ddaspit reviewed Apr 23, 2025

View reviewed changes

Enkidu93 assigned ddaspit Apr 30, 2025

isaac091 reviewed May 2, 2025

View reviewed changes

ddaspit force-pushed the update_block branch 5 times, most recently from 05c0f05 to fc9cbbc Compare May 7, 2025 18:23

johnml1135 added 4 commits May 7, 2025 15:09

Some tests pass

157ec48

Fix the tests

5b07325

I want to process the data in segments that correspond to individual …

d963c7a

…translations. These updates make it happen.

Added more test framework

7471207

johnml1135 added 6 commits May 7, 2025 15:09

Updates for reviewer comments

8a7c993

linting

92ee88e

Respond to reviewer comments

1fa8e82

Pass marker type (embed, style) to update block

Make last type - EMBED_BLOCK

bb730bf

linting

5ff5e07

Reviewer updates

9dba22b

ddaspit force-pushed the update_block branch from fc9cbbc to 76020a8 Compare May 7, 2025 20:10

Enkidu93 requested changes May 7, 2025

View reviewed changes

ddaspit force-pushed the update_block branch 2 times, most recently from e78cf2c to 447351c Compare May 7, 2025 22:30

ddaspit reviewed May 7, 2025

View reviewed changes

ddaspit force-pushed the update_block branch from 447351c to cbd54e3 Compare May 7, 2025 22:58

Refactor update block

402a03f

ddaspit force-pushed the update_block branch from cbd54e3 to 402a03f Compare May 7, 2025 23:19

ddaspit reviewed May 7, 2025

View reviewed changes

Enkidu93 approved these changes May 8, 2025

View reviewed changes

ddaspit approved these changes May 8, 2025

View reviewed changes

ddaspit merged commit 45f24a9 into main May 8, 2025
14 checks passed

ddaspit deleted the update_block branch May 8, 2025 21:59

This was referenced May 8, 2025

UpdateBlock data structure for updating pipeline sillsdev/serval#619

Closed

Multiple note texts in one embed sillsdev/serval#666

Closed

Enkidu93 mentioned this pull request May 27, 2025

Port update block sillsdev/machine#305

Merged

Uh oh!

Scripture Update block #168

Scripture Update block #168

Uh oh!

Conversation

johnml1135 commented Apr 3, 2025 • edited by ddaspit Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ddaspit left a comment

Choose a reason for hiding this comment

Uh oh!

johnml1135 commented Apr 10, 2025

Uh oh!

johnml1135 commented Apr 10, 2025

Uh oh!

johnml1135 commented Apr 10, 2025

Uh oh!

johnml1135 commented Apr 10, 2025

Uh oh!

johnml1135 commented Apr 10, 2025

Uh oh!

ddaspit left a comment

Choose a reason for hiding this comment

Uh oh!

johnml1135 commented Apr 11, 2025

Uh oh!

johnml1135 commented Apr 11, 2025

Uh oh!

johnml1135 commented Apr 11, 2025

Uh oh!

codecov-commenter commented Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ddaspit left a comment

Choose a reason for hiding this comment

Uh oh!

johnml1135 commented Apr 23, 2025

Uh oh!

johnml1135 commented Apr 23, 2025

Uh oh!

johnml1135 commented Apr 23, 2025

Uh oh!

johnml1135 commented Apr 23, 2025

Uh oh!

ddaspit left a comment

Choose a reason for hiding this comment

Uh oh!

isaac091 left a comment

Choose a reason for hiding this comment

Uh oh!

Enkidu93 left a comment

Choose a reason for hiding this comment

Uh oh!

ddaspit left a comment

Choose a reason for hiding this comment

Uh oh!

ddaspit left a comment

Choose a reason for hiding this comment

Uh oh!

Enkidu93 left a comment

Choose a reason for hiding this comment

Uh oh!

ddaspit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

johnml1135 commented Apr 3, 2025 •

edited by ddaspit

Loading

codecov-commenter commented Apr 14, 2025 •

edited

Loading