Hebrew ITN - Adding Hebrew ITN

[dankeinan1]

What does this PR do ?

Add Hebrew Inverse Text Normalization

Before your PR is "Ready for review"

Pre checks:

• Have you signed your commits? Use git commit -s to sign.
• Do all unittests finish successfully before sending PR?
  1. pytest or (if your machine does not have GPU) pytest --cpu from the root folder (given you marked your test cases accordingly @pytest.mark.run_only_on('CPU')).
  2. Sparrowhawk tests bash tools/text_processing_deployment/export_grammars.sh --MODE=test ...
• If you are adding a new feature: Have you added test cases for both pytest and Sparrowhawk here.
• Have you added __init__.py for every folder and subfolder, including data folder which has .TSV files?
• Have you followed codeQL results and removed unused variables and imports (report is at the bottom of the PR in github review box) ?
• Have you added the correct license header Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved. to all newly added Python files?
• If you copied nemo_text_processing/text_normalization/en/graph_utils.py your header's second line should be Copyright 2015 and onwards Google, Inc.. See an example here.
• Remove import guards (try import: ... except: ...) if not already done.
• If you added a new language or a new feature please update the NeMo documentation (lives in different repo).
• Have you added your language support to tools/text_processing_deployment/pynini_export.py.

PR Type:

• New Feature
• Bugfix
• Documentation
• Test

If you haven't finished some of the above items you can still open "Draft" PR.

[nirraviv89]

@tbartley94 this PR is now ready for review

[tbartley94]

Did a first pass and left initial comments for style and technical improvements. Very nice work. General suggestions:

1. Reuse prexisting graphs from the en.graph_utils. It's better to be conservative on these public variable in case there's waterfall effects from upstream pynini.

2. Our general style for graph composition is to aim for readability rather than compatness. If there's need for parentheses of depth > 1 , it's better to break the composition over several lines for readability. This is also important since Pynini is not always constant with order of operations when it comes to concatenation and union operations. So it's easier to break off rather than check set theory notes.

3. For compatibility with upstream sparrowhawk, you need to constrain the attributes you provide to graphs. Defined properties such as prefix can't be freely created when tagging. To change that requires creating a new fork of Sparrowhawk. Consult here for knowing which properties are usable per semiotic class:

https://github.com/google/sparrowhawk/blob/master/src/lib/semiotic_classes.pb.cc#L203-L216

I'll be pinging someone else to check the Hebrew outputs (not a reader). For clarity, should this be understood as a normalization system for colloquial use, or would it be more restricted to liturgical texts?

[tbartley94]

could you explain the multiple digit forms?

[dankeinan1]

Yes. The digits doesn't change there are different ways to say it. In Hebrew, numbers usually have gender, so we wanted to catch both masc. numbers and fem. numbers. In addition, some numbers are said differently like 2, when coming in construct state (proximity). For example "two hands" would be "שתי ידיים" and not " שתיים ידיים".
We wanted to include all the forms of the numbers, for better covering test cases.

[tbartley94]

Ah gotcha, and the infixation would make this a pain to just include the gendering in the graph. Could I bother you to split the data list into something like digit_masc digit_fem? It'll be easier for a maintainer to return to and understand without knowledge of the language.

[dankeinan1]

There is no easy way to include the gendering in the graph, because there is no basic rule to follow.
In addition, I think it would actually be more confusing and harder to maintain if we start splitting the digits to different lists, because they all used in the same places in the graphs. Maintaining different lists won't be easier in my opinion. I can do that, but I think that would not help maintainers in the future

[tbartley94]

It'd be preferred from a non-speaker end. Because of the amount of language knowledge that goes into these things, we want to make sure there's as much surface level knowledge available. It's just easier on my end to pass off to another maintainer this way.

[tbartley94]

Why isn't the whitelist written in the correct order in the first place? Will be difficult to track which graphs need to be inverted and which don't.

[dankeinan1]

What is the suggestion here? just to make sure I understand?
Invert the whitelist order so we do not need to use pynini.invert?

[tbartley94]

You're calling invert on the data file. Why not just have the original graph in the desired order.

[tbartley94]

same as with cardinals, this will break sparrowhawk.

https://github.com/google/sparrowhawk/blob/a0503e26a433fbd3a9ff81ba7a08819e4a3bb668/src/lib/semiotic_classes.pb.cc#L203-L216

[tbartley94]

will need to change due to sparrowhawk compatibility

[dankeinan1]

what is the required change? I am not sure I understand, can you please explain?

[bonham79]

day_prefix isn't a valid property you can pass with the date class tag. It will work for the Python codebase, but sparrowhawk deployment will be incompatible. this will be a massive thoroughput limitation.

[tbartley94]

sorry ^ is from me from my other git.

[dankeinan1]

We have to use prefixes in order to capture dates correctly.
What is your suggestion to fix this? Is the problem the variable name?
Why can't we add this prefix to the date graph?
I am not familiar enough with sparrowhawk to understand this comment. Please advise

[tbartley94]

Sorry, let me explain more clearly:

1. For Sparrowhawk, class table and property tags, in this case day_prefix, are a limited set. You can only use class labels and property tags defined in Sparrowhawk.

See here for permitted class tags: https://github.com/google/sparrowhawk/blob/a0503e26a433fbd3a9ff81ba7a08819e4a3bb668/src/lib/semiotic_classes.pb.cc#L24-L59

See here for permitted property tags for date:

https://github.com/google/sparrowhawk/blob/a0503e26a433fbd3a9ff81ba7a08819e4a3bb668/src/lib/semiotic_classes.pb.cc#L205-L216

2. While the labels are closed, what is allowed within a given label is not closed. So you can use any arbitrary property you want. So you can replace day_prefix with style or morphosyntactic_feature in any place you use day_prefix. It just needs to be regular and managed by the verbalizer. My personal choice would just be to use morphosyntactic_feature since that's the general purpose for that tag. If there's multiple prefixes you're trying to manage, you can just use a trivial delimiter. (e.g. morphosyntactic_feature: DAY_PREFIX/OTHER_PREFIX)

3. Because Sparrowhawk is archived, this cannot be changed upstream without a fork. So it's a hard restriction (barring you want to create a PR for an archived repo....)

4. This restriction is only for Sparrowhawk. The NeMo Text Processing library proper allows trivial tags. So if you believe the change is outside the scope of the PR, you can let me know and I'll create an issue for Sparrowhawk integration with my team.