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

Remove hardcoded segmenter data from datagen #3003

Closed
robertbastian opened this issue Jan 18, 2023 · 14 comments
Closed

Remove hardcoded segmenter data from datagen #3003

robertbastian opened this issue Jan 18, 2023 · 14 comments
Assignees
Labels
A-data Area: Data coverage or quality C-segmentation Component: Segmentation S-medium Size: Less than a week (larger bug fix or enhancement)

Comments

@robertbastian
Copy link
Member

robertbastian commented Jan 18, 2023

There are many data files located here:

https://github.com/unicode-org/icu4x/tree/main/provider/datagen/data/segmenter

Is this the best place for the source of truth, or can we source them from elsewhere?

@robertbastian robertbastian added C-segmentation Component: Segmentation S-medium Size: Less than a week (larger bug fix or enhancement) A-data Area: Data coverage or quality labels Jan 18, 2023
@sffc sffc added this to the 1.2 Blocking ⟨P1⟩ milestone Jan 19, 2023
@sffc
Copy link
Member

sffc commented Feb 8, 2023

@aethanyc --- could you take a look at these files and list where you think appropriate data sources would be?

@aethanyc
Copy link
Contributor

aethanyc commented Feb 9, 2023

LSTM models is from https://github.com/unicode-org/lstm_word_segmentation/tree/develop/Models. We can download them if they are packed in lstm_word_segmentation repository. Note: they are currently only available in the develop branch, but not in the main branch.


Dictionary toml files are converted via the following command. (See the comment in the beginning of each toml file.) For example, CJ dictionary:

# This data is created by the following using ICU4C tools
# LD_LIBRARY_PATH=lib bin/gendict --uchars data/brkitr/dictionaries/cjdic.txt tmp.bin
# dd if=tmp.bin of=cjdict.dict bs=1 skip=64

Maybe the conversion and packing can be part of the ICU4C release process so that we can download it somewhere?


UAX14 rules are implemented ourselves in line.toml, and UAX29 rules in grapheme.toml, sentence.toml, and word.toml. These toml files are written by hand, not derived from other files. They are the source of truth, and should live in ICU4X.

cc @makotokato to double check my knowledge.

@makotokato
Copy link
Member

Correct. #2519 for char16trie data generation.

@aethanyc
Copy link
Contributor

@robertbastian @sffc Per my comment above, it is nice to have the LSTM and dictionary data download/generated from somewhere, so this issue seems like a P3 or P4 to me. Does it have to be P1 to block the release?

@sffc
Copy link
Member

sffc commented Feb 22, 2023

I guess my main concern is that this involves adding additional sources to datagen, so it would be best to have those in place when people start using datagen for segmenter.

@Manishearth
Copy link
Member

cc @eggrobin this is the issue, which has multiple parts:

  • We hardcode rule-based data (manually written)
  • We hardcode dictionary data (generated from icu4c, probably should be in icuexportdata)
  • We hardcode LSTM data (comes from a different repo)

@robertbastian
Copy link
Member Author

@sffc
Copy link
Member

sffc commented Apr 27, 2023

Discussions:

  • For the manually written rule tables, we should consider upstreaming them. They are basically machine-readable versions of UAX 14/29. CC @FrankYFTang for thoughts on that.
  • For the dictionary data, it makes sense to add it to icuexportdata. We will probably keep the hardcoded dictionary tables in datagen until 2.0 to avoid breakages.
  • For the LSTM data, make a release on the LSTM repo with the JSON files. This is forwards compatible except for the no_default_features mode of icu4x-datagen. Probably keep the LSTM data in datagen too, until 2.0.

@robertbastian
Copy link
Member Author

With #3396 and #3399 dictionary and LSTM sources are now controlled by the client. However, we will keep the hardcoded fallback data around until 2.0.

What's left are the handwritten rule tables.

@robertbastian robertbastian self-assigned this May 10, 2023
@robertbastian robertbastian added discuss-priority Discuss at the next ICU4X meeting and removed discuss Discuss at a future ICU4X-SC meeting labels May 10, 2023
@sffc
Copy link
Member

sffc commented May 11, 2023

Discussion: we could upstream the data files into CLDR but we need to make sure they are easily maintainable.

@sffc
Copy link
Member

sffc commented May 11, 2023

@robertbastian to drive the relationship with CLDR to get these files upstreamed.

@robertbastian
Copy link
Member Author

So there are segmentation files in CLDR already, which are generated from https://github.com/unicode-org/unicodetools/blob/main/unicodetools/src/main/resources/org/unicode/tools/SegmenterDefault.txt (this is a lot closer to the spec and might become part of the spec in the future). Given that the data is already there, I think it's unlikely that our (more processed) TOML versions will get accepted.

I've added parsing for these CLDR files in #3440, but I will need @makotokato's help to generate our representation from those.

@sffc sffc removed the discuss-priority Discuss at the next ICU4X meeting label May 25, 2023
@robertbastian
Copy link
Member Author

Closing this for now as LSTM and dictionary data is done, and rules is a bigger Unicode-wide undertaking for Q3.

@sffc
Copy link
Member

sffc commented May 25, 2023

Follow-up: #3457

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-data Area: Data coverage or quality C-segmentation Component: Segmentation S-medium Size: Less than a week (larger bug fix or enhancement)
Projects
None yet
Development

No branches or pull requests

6 participants