Add new Datagen API and CLI with FallbackOptions #4710

sffc · 2024-03-19T22:51:12Z

sffc · 2024-03-19T23:13:35Z

This PR doesn't export any APIs, but it plumbs the old APIs through the new APIs with no detectable output diffs. I can merge this now or keep working in this PR.

provider/datagen/src/driver.rs

robertbastian · 2024-03-20T13:26:25Z

provider/datagen/src/driver.rs

+            return false;
+        };
+        let mut it = config.locales.iter();
+        matches!((it.next(), it.next()), (Some(lid), None) if lid == &LocaleWithExpansion::wildcard())


if you make wildcard() an associated const you can match against it.

For now I'd rather keep it a function because it's more flexible.

provider/datagen/src/driver.rs

(cherry picked from commit c1d463a)

(cherry picked from commit c46a141)

(cherry picked from commit ab745ff)

provider/datagen/src/driver.rs

robertbastian · 2024-03-25T13:11:14Z

provider/datagen/src/driver.rs

    pub runtime_fallback_location: Option<RuntimeFallbackLocation>,
    pub deduplication_strategy: Option<DeduplicationStrategy>,
-    /// private: set via constructor
-    locales: HashSet<LocaleWithExpansion>,
+    pub und_inclusion: Option<UndInclusion>,


Why are you special-casing und? It will either be included due to fallback, or the user can include it through @und.

Good question; I wanted to make it easier to exclude und, which is desirable when building language packs. (Frank was having this problem building a language pack for Chrome.) Strictly speaking, this can be modeled as a LocaleFamily variant, such as "ancestors and descendants except for und". However, I don't see a super clean way to handle it. The options would be:

--locales en-001 implies und, en, ... and the user needs to specify --without-und to disable und (what this changelist implies)

Pros: Puts the special case of und as its own flag for users who need this behavior

Cons: Entanglement of options

--locales en-001 implies en, ..., (remove und from LocaleFamily with_ancestors) and --with-und (default behavior) includes und, and the user can specify --without-und to disable the inclusion of und

Pros: A cleaner model with less conflict between options

Cons: Still an extra flag to imply this behavior

--locales en-001 implies und, en, ..., and a new LocaleFamily type _en-001 implies en, ...

Pros: Clean model with all locale resolution handling in the same mechanism

Cons: Every locale needs to be prefixed with _ in order to avoid including und. Need to also invent a syntax for the "ancestors, no descendants, no und" mode.

--locales en-001 implies und, en, ..., and a new LocaleFamily type !und removes und from the selection

Pros: All locale resolution happens in the same option. New include/exclude syntax could be used to model other things.

Cons: Include/exclude syntax could be error-prone. We now have an ordered list of rules instead of an unordered set of families.

Do you see anything else?

I was expecting we'd do something like 3. Including whole locale families but excluding und is a niche (and risky) enough behaviour that introducing a new locale family symbol shouldn't be the main problem. If the main use case is language packs, there'd usually only be a single und-less family per invocation anyway.

I don't understand why we need a "ancestors, no descendants, no und" mode. und is an ancestor, and it's not special. If you're building en language packs, you don't want to duplicate en-001 the same way you don't want to duplicate und.

Will do in a follow-up

provider/datagen/src/driver.rs

sffc · 2024-03-25T15:20:28Z

provider/datagen/src/driver.rs

    pub runtime_fallback_location: Option<RuntimeFallbackLocation>,
    pub deduplication_strategy: Option<DeduplicationStrategy>,
-    /// private: set via constructor
-    locales: HashSet<LocaleWithExpansion>,
+    pub und_inclusion: Option<UndInclusion>,


Good question; I wanted to make it easier to exclude und, which is desirable when building language packs. (Frank was having this problem building a language pack for Chrome.) Strictly speaking, this can be modeled as a LocaleFamily variant, such as "ancestors and descendants except for und". However, I don't see a super clean way to handle it. The options would be:

--locales en-001 implies und, en, ... and the user needs to specify --without-und to disable und (what this changelist implies)

Pros: Puts the special case of und as its own flag for users who need this behavior

Cons: Entanglement of options

--locales en-001 implies en, ..., (remove und from LocaleFamily with_ancestors) and --with-und (default behavior) includes und, and the user can specify --without-und to disable the inclusion of und

Pros: A cleaner model with less conflict between options

Cons: Still an extra flag to imply this behavior

--locales en-001 implies und, en, ..., and a new LocaleFamily type _en-001 implies en, ...

Pros: Clean model with all locale resolution handling in the same mechanism

Cons: Every locale needs to be prefixed with _ in order to avoid including und. Need to also invent a syntax for the "ancestors, no descendants, no und" mode.

--locales en-001 implies und, en, ..., and a new LocaleFamily type !und removes und from the selection

Pros: All locale resolution happens in the same option. New include/exclude syntax could be used to model other things.

Cons: Include/exclude syntax could be error-prone. We now have an ordered list of rules instead of an unordered set of families.

Do you see anything else?

robertbastian · 2024-03-25T15:32:23Z

provider/datagen/src/driver.rs

    pub runtime_fallback_location: Option<RuntimeFallbackLocation>,
    pub deduplication_strategy: Option<DeduplicationStrategy>,
-    /// private: set via constructor
-    locales: HashSet<LocaleWithExpansion>,
+    pub und_inclusion: Option<UndInclusion>,


I was expecting we'd do something like 3. Including whole locale families but excluding und is a niche (and risky) enough behaviour that introducing a new locale family symbol shouldn't be the main problem. If the main use case is language packs, there'd usually only be a single und-less family per invocation anyway.

I don't understand why we need a "ancestors, no descendants, no und" mode. und is an ancestor, and it's not special. If you're building en language packs, you don't want to duplicate en-001 the same way you don't want to duplicate und.

robertbastian · 2024-03-25T15:33:43Z

provider/datagen/src/driver.rs

+    /// Sets this driver to generate the given locales assuming runtime fallback.
+    ///
+    /// Use the [`langid!`] macro from the prelude to create an
+    /// explicit list, or [`DatagenProvider::locales_for_coverage_levels`] for CLDR coverage levels.


...or create language families

sffc · 2024-03-26T15:44:10Z

Plan for this PR:

Keep the (verbose) API agreed on in the issue
Remove include_und option
OK to add LocaleFamily::without_ancestors but could be in a follow-up

LGTM: @robertbastian

provider/datagen/src/bin/datagen/args.rs

robertbastian · 2024-03-27T10:51:52Z

ffi/dart/tools/datagen/src/main.rs

-        .map(|l| l.parse::<LanguageIdentifier>())
+        .map(|l| {
+            l.parse::<LanguageIdentifier>()
+                .map(LocaleFamily::with_descendants)


I assume you're doing this in a follow-up: here, as on the main CLI, we should parse directly to LocaleFamily.

Thanks for the reminder. Working on it.

I think you fixed it in datagen but not here?

Ah no you didn't change it in datagen, that's why CI is failing

provider/datagen/src/bin/datagen/mod.rs

provider/datagen/src/driver.rs

robertbastian · 2024-03-27T11:21:30Z

provider/datagen/src/driver.rs

+                let mut sorted_locales =
+                    locales.iter().map(ToString::to_string).collect::<Vec<_>>();
+                sorted_locales.sort();


Found something cool for this: https://internals.rust-lang.org/t/add-a-vec-sorted-function/11948/3. We already have itertools in datagen

Note that it returns std::vec::IntoIter, so it still does the same thing under the hood, just a bit nicer syntax and a bit more prone to error because if you call any other iterator methods like .map() then you end up double-allocating. It also seems a bit risky that std::vec::IntoIter::collect uses specialization. But I can do this if you like.

I just think it's nicer because it avoids an intermediate variable.

not blocking

ffi/dart/tools/datagen/build.rs

sffc · 2024-03-27T15:58:01Z

@robertbastian CI is passing as of a245cbb. I started adding the LocaleFamily string parsing logic (thanks for the reminder) but don't have it quite ready yet. Do you approve this pending the remainder of the internal plumbing for LocaleFamily in the CLI? Also, if you prefer I can roll back to a245cbb (or c5ddea3, which has some renames and hopefully passes CI) and merge that and put the LocaleFamily parsing in a follow-up.

robertbastian · 2024-03-27T16:00:46Z

I approve at c5ddea3, and also the rest if you get CI to pass.

robertbastian · 2024-03-27T16:37:03Z

provider/datagen/src/bin/datagen/mod.rs

@@ -94,7 +94,7 @@ fn main() -> eyre::Result<()> {
    let locales_intermediate: LanguageIdentifiersOrLocaleFamilies = match config.locales {
        config::LocaleInclude::All => LocaleFamilies(vec![LocaleFamily::full()]),
        config::LocaleInclude::None => LanguageIdentifiers(vec![]),
-        config::LocaleInclude::Explicit(set) => LanguageIdentifiers(set.into_iter().collect()),
+        config::LocaleInclude::Explicit(set) => LocaleFamilies(set.into_iter().collect()),
        config::LocaleInclude::CldrSet(levels) => LanguageIdentifiers(


thought: I think we've been treating cldr sets as families

This locales_intermediate just preserves the type things come in as. They get converted to LocaleFamilies in with_fallback and they stay as LanguageIdentifiers in without_fallback.

sffc · 2024-03-27T16:56:40Z

provider/datagen/src/bin/datagen/mod.rs

+            LocaleFamilies(lfs) => lfs
+                .into_iter()
+                .map(|family| family.write_to_string().parse())
+                .collect::<Result<Vec<LanguageIdentifier>, icu_locid::ParserError>>()?,


Note: This logic is a bit strange but I think it gets the job done. In Clap we parse the list to a Vec<LocaleFamily>, and then here I re-parse it as a Vec<LanguageIdentifier>. I guess another way to do this would be to parse to a Vec<String> in Clap and then pick the concrete type here.

sffc · 2024-03-27T17:18:52Z

This is ready to land. @robertbastian last approved it modulo making CI/tests pass, which is now the case.

sffc added 6 commits March 19, 2024 23:20

Checkpoint: new APIs plumbed through. Need to test.

a6bdfce

Make tests pass

1f19ba4

Don't impl Display on public types unless we mean it

5b2dcd7

Fix defaults; make DeduplicationStrategy optional

6f93495

fmt

ace5856

clippy and docs

d323114

sffc marked this pull request as ready for review March 19, 2024 23:12

sffc requested review from robertbastian and Manishearth as code owners March 19, 2024 23:12

robertbastian reviewed Mar 20, 2024

View reviewed changes

Manishearth removed their request for review March 21, 2024 09:53

sffc added 13 commits March 21, 2024 17:40

Rename and simplify some things

5e13943

Refactor code to remove coerce functions

ec2807a

Add more DatagenDriver options tests

b2352e3

(cherry picked from commit c1d463a)

Fix UndInclusion and wildcard

ce406c8

Add empty list test

4002457

(cherry picked from commit c46a141)

fmt

d69a9ec

'full' instead of '*'

1c477c2

Consolidate all-locales handling into main loop

b25f974

Add docs and export the types

24efc92

Add 1.5 API to tests and mark old API deprecated

2b5268a

LocaleFamily and docs

a70b929

Add PreferredForExporter test

7e1cb29

(cherry picked from commit ab745ff)

Add the 1.5 default options test

0ee50e6

robertbastian reviewed Mar 25, 2024

View reviewed changes

sffc commented Mar 25, 2024

View reviewed changes

robertbastian reviewed Mar 25, 2024

View reviewed changes

Merge branch 'main' into datagen-neo-driver

60f78e1

sffc requested a review from a team as a code owner March 26, 2024 22:17

Fix test failure in blob provider. Needs investigation

ed497a0

sffc changed the title ~~Add plumbing for LocalesWithFallback Datagen API~~ Add new Datagen API and CLI with FallbackOptions Mar 27, 2024

sffc added 4 commits March 26, 2024 18:13

doc

527d4c0

Better fix for the blob provider test failure

378fb93

Refactoring

62e75b5

Consolidate types and other nits

4f51fa9

robertbastian reviewed Mar 27, 2024

View reviewed changes

sffc added 6 commits March 27, 2024 08:00

Maybe fix CI

28c0ef5

oops

4a76bf6

Improve logging

a245cbb

Fix CLI message

78e29ac

NoDeduplication > None

c5ddea3

LocaleFamily serialization and stuff

8460810

sffc mentioned this pull request Mar 27, 2024

Implement Datagen API LocaleExpansionMode #4629

Closed

sffc requested a review from robertbastian March 27, 2024 15:55

Fix build?

5474401

robertbastian previously approved these changes Mar 27, 2024

View reviewed changes

sffc added 2 commits March 27, 2024 09:46

Testing

1cccda7

Remap LocaleFamilies to LanguageIdentifiers in without-fallback

5bebfbc

sffc dismissed robertbastian’s stale review via 5bebfbc March 27, 2024 16:52

sffc commented Mar 27, 2024

View reviewed changes

robertbastian approved these changes Mar 27, 2024

View reviewed changes

sffc merged commit 7397d7d into unicode-org:main Mar 27, 2024
30 checks passed

sffc deleted the datagen-neo-driver branch March 27, 2024 20:33

sffc removed the request for review from a team March 27, 2024 20:33

sffc mentioned this pull request Apr 10, 2024

Add BaseLanguageHandling option to Datagen #4606

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new Datagen API and CLI with FallbackOptions #4710

Add new Datagen API and CLI with FallbackOptions #4710

sffc commented Mar 19, 2024

sffc commented Mar 19, 2024

robertbastian Mar 20, 2024

sffc Mar 27, 2024

robertbastian Mar 25, 2024

sffc Mar 25, 2024

robertbastian Mar 25, 2024

sffc Mar 27, 2024

sffc Mar 25, 2024

robertbastian Mar 25, 2024

robertbastian Mar 25, 2024

sffc commented Mar 26, 2024

robertbastian Mar 27, 2024

sffc Mar 27, 2024

robertbastian Mar 27, 2024

robertbastian Mar 27, 2024

robertbastian Mar 27, 2024

sffc Mar 27, 2024

robertbastian Mar 27, 2024

robertbastian Mar 27, 2024

sffc commented Mar 27, 2024 •

edited

Loading

robertbastian commented Mar 27, 2024

robertbastian Mar 27, 2024

sffc Mar 27, 2024

sffc Mar 27, 2024

sffc commented Mar 27, 2024

Add new Datagen API and CLI with FallbackOptions #4710

Add new Datagen API and CLI with FallbackOptions #4710

Conversation

sffc commented Mar 19, 2024

sffc commented Mar 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Mar 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Mar 27, 2024 • edited Loading

robertbastian commented Mar 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Mar 27, 2024

sffc commented Mar 27, 2024 •

edited

Loading