Datagen CLI improvements #2950

robertbastian · 2023-01-04T18:45:37Z

I made more parameters have defaults so that it's easier to play around with:

--keys all for all keys, --keys none for none (already available through the API)
--locales full for all locales, --locales none for none (now works correctly on the API level, before it would still generate unds)
--format now prints a warning when omitted
--cldr-tag and --icuexport-tag now default to latest. Downloading is lazy now though, so we'll only talk to the network if the data is needed (i.e. not if we do --locales none or --keys none)
databake now fails if the directory exists. We cannot add an overwrite flag because that'd be breaking
--out now defaults to icu4x_data for --format=dir and --format=mod
Removed --hello-world. It's mainly used internally, and now that all keys work out of the box, clients shouldn't have a need for this

This means the invocation

icu4x-datagen --format=mod --keys all --locales full

is now enough to generate data for all locales, all keys, with the latest CLDR and ICU data.

Similarly

icu4x-datagen --format=mod --keys all --locales none

now also runs without downloading any data, and can be used to generate initial baked data to make a binary (or, stay tuned, an FFI layer, #2743) compile.

sffc

There are a lot of good things in here, and other things that need further discussion.

Note that I see #2945 as the path forward to better support clients who want easy, default settings, so I put less value on the ergonomics of icu4x-datagen. I put more value on the educational value of icu4x-datagen.

docs/tutorials/data_management.md

provider/datagen/src/bin/datagen.rs

sffc · 2023-01-05T21:19:16Z

provider/datagen/src/bin/datagen.rs

        )
        .arg(
            Arg::with_name("CLDR_TAG")
                .short("t")
                .long("cldr-tag")
                .value_name("TAG")
+                .default_value("latest")


Discussion on setting defaults for --cldr-tag and --icuexport-tag

Pros and cons for making this change:

Pro: This is the option most people should choose if they don't know any better.

Con: It makes the default behavior of icu4x-datagen be non-deterministic and network-dependent, which seems undesirable.

Con: It's good for educational purposes for people to make an explicit decision about where to get their data.

Con: It makes the default behavior of icu4x-datagen be non-deterministic and network-dependent, which seems undesirable.

I see icu4x-datagen as a simple tool to get started with ICU4X, so it should be allowed to use the network by default. Note that the API does not download by default, which is more appropriate for build scripts.

Con: It's good for educational purposes for people to make an explicit decision about where to get their data.

I don't think we should encourage non-standard or outdated data sources. If clients want to use the binary in some kind of build process, they will look at icu4-datagen --help.

If others agree that network access in the CLI version of icu4x-datagen is OK as a default, I'm fine with this.

@zbraniecki @Manishearth

Merging this because it blocks another PR, we can address this in a follow-up.

provider/datagen/src/bin/datagen.rs

provider/datagen/src/source.rs

provider/testdata/src/bin/datagen.rs

tools/scripts/data.toml

provider/datagen/src/lib.rs

sffc · 2023-01-05T23:02:00Z

I'm almost sold on setting the default CLDR/ICU to latest, but for now I still prefer users to write an explicit --all-keys --all-locales. It makes a nice and clear symmetry to show what's going on. I think it's fair to say that these are the 2 most important flags, and they can be required. --format used to be defaulted, and now it's also required, which seems fine.

sffc · 2023-01-06T00:27:27Z

docs/tutorials/data_management.md

@@ -23,18 +23,13 @@ Get a coffee, this might take a while ☕.
 Once installed, run:

 ```console
-$ icu4x-datagen --cldr-tag latest --icuexport-tag latest --out my_data_blob.postcard --format blob --all-keys --all-locales
+$ icu4x-datagen --keys all --locales all --format blob --out my_data_blob.postcard


Possible Issue: all is a valid locale, Allar (none is fine because it is 4 letters)

Bikeshed:

--locales all and bet that there won't ever be Allar data in CLDR (this might not be a very safe bet, because CLDR is working on adding seed data in thousands of locales for things like "default region for language", etc)

--locales-all

--locales full

--locales every

--locales all!

or stick with --all-locales

Yes I thought about this, but it's very unlikely that a client would want to generate just Allar, don't you think? If they're generating many single-locale blobs, they should use the API. I think it's fine to say generating only Allar is not supported by the CLI.

Maybe any is actually a better name, because it is actually a filter. Also exists though.

The reason why I want to get rid of the separate --all-x arguments is because they clutter the --help output.

I don't find that to be a huge concern, personally. Not really a fan of ambiguity in the format

Why not --locales *?

@macchiati pointed out that these are not actually "all" locales; they are only whatever locales happen to be covered in the current CLDR release. So, --locales modern or --locales basic could be more precise.

@srl295 suggested that keywords could be expected to be wrapped with some other characters, like --locales <none> or --locales "*".

@pedberg-icu agrees that we "should not use anything that could be confused with a potential language code"

What's --locales basic? PTAL

The CLDR coverage levels are basic, moderate, and modern. I think full and basic correspond to the same set of locales?

The CLDR JSON sets are probably modern = {modern} and full = {modern, moderate, basic} then?

The CLDR JSON sets are probably modern = {modern} and full = {modern, moderate, basic} then?

I believe so; @srl295 can you confirm?

provider/datagen/src/bin/datagen.rs

sffc · 2023-01-06T00:41:40Z

docs/tutorials/data_management.md

@@ -23,18 +23,13 @@ Get a coffee, this might take a while ☕.
 Once installed, run:

 ```console
-$ icu4x-datagen --cldr-tag latest --icuexport-tag latest --out my_data_blob.postcard --format blob --all-keys --all-locales
+$ icu4x-datagen --keys all --locales all --format blob --out my_data_blob.postcard


Bikeshed:

--locales all and bet that there won't ever be Allar data in CLDR (this might not be a very safe bet, because CLDR is working on adding seed data in thousands of locales for things like "default region for language", etc)

--locales-all

--locales full

--locales every

--locales all!

or stick with --all-locales

provider/datagen/src/source.rs

provider/datagen/src/bin/datagen.rs

.github/workflows/build-test.yml

jira-pull-request-webhook · 2023-01-09T18:17:48Z

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

robertbastian force-pushed the lazysource branch from 781b0d2 to 117b4ff Compare January 4, 2023 21:14

unicode-org deleted a comment from jira-pull-request-webhook bot Jan 4, 2023

robertbastian force-pushed the lazysource branch from 117b4ff to ad075f0 Compare January 4, 2023 21:54

unicode-org deleted a comment from jira-pull-request-webhook bot Jan 4, 2023

robertbastian force-pushed the lazysource branch from ad075f0 to e3122d1 Compare January 4, 2023 22:27

lazysource

dc2d8f2

robertbastian force-pushed the lazysource branch from e3122d1 to dc2d8f2 Compare January 4, 2023 23:14

unicode-org deleted a comment from jira-pull-request-webhook bot Jan 4, 2023

robertbastian marked this pull request as ready for review January 4, 2023 23:31

robertbastian requested review from Manishearth, sffc and a team as code owners January 4, 2023 23:31

robertbastian added 2 commits January 5, 2023 13:58

compiling empty any

0ff6bca

fmt

60ef306

robertbastian mentioned this pull request Jan 5, 2023

FFI baked data #2947

Closed

sffc requested changes Jan 5, 2023

View reviewed changes

feedback

958ce4c

robertbastian force-pushed the lazysource branch from 39b541a to 958ce4c Compare January 5, 2023 23:38

unicode-org deleted a comment from jira-pull-request-webhook bot Jan 5, 2023

nit

a773a37

robertbastian requested a review from sffc January 5, 2023 23:48

fix

d76d17c

sffc reviewed Jan 6, 2023

View reviewed changes

feedback

cc7d929

robertbastian requested a review from sffc January 6, 2023 14:43

robertbastian added 2 commits January 6, 2023 15:48

doc

3c82337

fmt

4f7e146

robertbastian added 6 commits January 6, 2023 16:29

fix

83a7105

fix

800e021

fix

b27c335

rm hardcoded tags

01d4559

fix

24c06de

ugh

11daf7f

sffc reviewed Jan 6, 2023

View reviewed changes

provider/datagen/src/bin/datagen.rs Show resolved Hide resolved

.github/workflows/build-test.yml Show resolved Hide resolved

Merge branch 'main' into lazysource

52259bf

robertbastian requested a review from sffc January 6, 2023 20:27

fullmodern

0454e0c

robertbastian force-pushed the lazysource branch from 9162b25 to 0454e0c Compare January 9, 2023 18:17

tidy

fe81315

sffc approved these changes Jan 9, 2023

View reviewed changes

Manishearth approved these changes Jan 10, 2023

View reviewed changes

robertbastian merged commit 15d8e3a into unicode-org:main Jan 10, 2023

robertbastian deleted the lazysource branch January 10, 2023 10:31

robertbastian mentioned this pull request Jan 23, 2023

1.1.0 #3018

Merged

Datagen CLI improvements #2950

Datagen CLI improvements #2950

Uh oh!

Conversation

robertbastian commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sffc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Discussion on setting defaults for --cldr-tag and --icuexport-tag

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sffc commented Jan 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sffc Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sffc Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jira-pull-request-webhook bot commented Jan 9, 2023

Uh oh!

Uh oh!

robertbastian commented Jan 4, 2023 •

edited

Loading

Discussion on setting defaults for `--cldr-tag` and `--icuexport-tag`

sffc Jan 6, 2023 •

edited

Loading

sffc Jan 6, 2023 •

edited

Loading