Adding all keys to testdata #3042

robertbastian · 2023-01-30T18:25:07Z

icu_testdata currently doesn't include a handful of properties keys. It also doesn't include some collations for locales that it includes other data in.

Manishearth · 2023-01-30T18:39:14Z

Deferring to Shane on this, he has more context

.github/workflows/build-test.yml

tools/testdata-scripts/src/bin/make-testdata.rs

provider/blob/benches/provider_blob.rs

sffc · 2023-01-31T16:36:04Z

tools/testdata-scripts/src/bin/make-testdata.rs

@@ -34,9 +14,9 @@ fn main() {
        .unwrap();

    let source_data = SourceData::default()
-        .with_cldr(paths::cldr_json_root(), CldrLocaleSubset::Full)
+        .with_cldr_for_tag(&icu_testdata::versions::cldr_tag(), CldrLocaleSubset::Full)


Issue: Please, please make this read from the repo. A dev should be able to run cargo make testdata without an internet connection. This has always worked and there's no reason to break it.

The repo might not reflect the correct data for this tag. This is more of an issue for the follow-up PR, where we generate testdata at SourceData::LATEST_TESTED_CLDR_TAG and then write that tag into the metadata. Local data could be wrong because someone forgot a glob, or we just haven't run the download script with the latest tag. I'm also more confident calling the constant LATEST_TESTED_CLDR_TAG if we actually test with it.

"Download" has been required for many things locally and on CI for quite a while now. Because it caches it's a one-time thing and we haven't had issues with it.

The solution we've discussed is that there should be a CI action that verifies that the repo data is not stale.

"Download" has been required for many things locally and on CI for quite a while now.

CI yes, but we've kept local development as no-network as possible.

It's also much easier to reason about the data pipeline if I can see exactly what data is being consumed by datagen in order to produce the processed ICU4X output data. Depending on some artifact downloaded from the network is simply not a good idea for the purposes of ICU4X development.

I'd argue relying on the network is much less of a problem locally than in CI. Then work-on-plane use case is rare, and even then you're gonna have the data in the cache unless you started ICU4X development just before boarding.

Do you disagree with my point about calling the latest-tested-tag tested?

tools/testdata-scripts/src/bin/make-testdata.rs

sffc · 2023-01-31T17:01:49Z

Also remember that I was reluctant to go along with the user-facing version of icu4x-datagen downloading its data automatically; I agreed to that after weighing the following advantages: (1) icu4x-datagen has a lot of flags and we want to make it more accessible, since it's one of the first things ICU4X users need to use, and (2) as we add more data sources, we need to keep older invocations of icu4x-datagen working properly, which means we need to give them default sources.

Neither of those points for icu4x-datagen applies here in testdata datagen.

robertbastian · 2023-01-31T17:07:11Z

What's your preferred solution to the final testdata script then?

icu4x/tools/testdata-scripts/src/bin/make-testdata.rs

Lines 27 to 29 in c2a5392

    
           .with_cldr_for_tag(SourceData::LATEST_TESTED_CLDR_TAG, CldrLocaleSubset::Full) 
        
           .unwrap() 
        
           .with_icuexport_for_tag(SourceData::LATEST_TESTED_ICUEXPORT_TAG)

icu4x/tools/testdata-scripts/src/bin/make-testdata.rs

Lines 89 to 90 in c2a5392

    
           let cldr_tag = SourceData::LATEST_TESTED_CLDR_TAG; 
        
           let icu_tag = SourceData::LATEST_TESTED_ICUEXPORT_TAG;

Note that we generate testdata for a tag and then write that tag to metadata. Do we just pray that the local data is what would have been downloaded for the tag?

sffc · 2023-01-31T17:11:37Z

There should be a CI that verifies that the copy of the data in the repo corresponds to the tag.

sffc · 2023-01-31T17:12:29Z

See #2880

robertbastian · 2023-01-31T17:18:04Z

This is where I fundamentally disagree, keeping state in the repo and having CI verify that the state is correct adds a lot of friction when the state is easy to regenerate. I already filed #3006 about this.

robertbastian · 2023-01-31T17:24:01Z

Also the data will not correspond to the tag, because we only include select files. That itself could hide bugs.

sffc · 2023-01-31T17:29:53Z

This is where I fundamentally disagree, keeping state in the repo and having CI verify that the state is correct adds a lot of friction when the state is easy to regenerate. I already filed #3006 about this.

I see the friction argument for things that change "unexpectedly", like the GN files. But, these are files that don't change often, and when they do, it's normally intentional. If they get out of sync, that's a mistake, not an oversight.

Also the data will not correspond to the tag, because we only include select files. That itself could hide bugs.

Can you elaborate on the types of bugs this creates?

robertbastian · 2023-01-31T17:36:48Z

If they get out of sync, that's a mistake, not an oversight.

I don't understand the difference and how that changes things.

Can you elaborate on the types of bugs this creates?

A new file gets added to icuexport but not to the glob. This file breaks some assumption in datagen (which often lists directories) and breaks it.

sffc

It doesn't make testdata smaller, but if adding all keys is the desired outcome, I like this way of doing it

robertbastian · 2023-02-01T18:53:31Z

Testdata should contain all keys so that it works with all constructors. For collation maybe it's including too much, but we have to eat our own dogfood here, datagen users cannot at the moment exclude things like und-u-co-emoji.

robertbastian requested review from Manishearth, sffc and a team as code owners January 30, 2023 18:25

Manishearth removed request for a team and Manishearth January 30, 2023 18:39

robertbastian mentioned this pull request Jan 30, 2023

Moving source data out of icu_testdata #3044

Merged

sffc requested changes Jan 30, 2023

View reviewed changes

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

tools/testdata-scripts/src/bin/make-testdata.rs Outdated Show resolved Hide resolved

provider/blob/benches/provider_blob.rs Outdated Show resolved Hide resolved

fulltest

78843a4

robertbastian force-pushed the fulltest branch from 92ea07e to 78843a4 Compare January 30, 2023 19:50

This comment was marked as spam.

Sign in to view

robertbastian requested a review from sffc January 30, 2023 19:51

sffc requested changes Jan 31, 2023

View reviewed changes

robertbastian requested a review from sffc January 31, 2023 16:48

sffc requested changes Jan 31, 2023

View reviewed changes

tools/testdata-scripts/src/bin/make-testdata.rs Outdated Show resolved Hide resolved

robertbastian changed the title ~~Adding all keys to testdata~~ Make testdata datagen work with latest git tags Jan 31, 2023

local data

8b0b7be

robertbastian changed the title ~~Make testdata datagen work with latest git tags~~ Adding all keys to testdata Feb 1, 2023

robertbastian requested a review from sffc February 1, 2023 11:32

ci

3f61993

sffc approved these changes Feb 1, 2023

View reviewed changes

robertbastian merged commit 8559789 into unicode-org:main Feb 1, 2023

robertbastian mentioned this pull request Feb 9, 2023

Separating HelloWorldProvider and icu_testdata #3096

Closed

robertbastian deleted the fulltest branch February 9, 2023 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding all keys to testdata #3042

Adding all keys to testdata #3042

robertbastian commented Jan 30, 2023 •

edited

Loading

Manishearth commented Jan 30, 2023

This comment was marked as spam.

sffc Jan 31, 2023

robertbastian Jan 31, 2023

sffc Jan 31, 2023

robertbastian Jan 31, 2023

robertbastian Jan 31, 2023

sffc commented Jan 31, 2023

robertbastian commented Jan 31, 2023 •

edited

Loading

sffc commented Jan 31, 2023

sffc commented Jan 31, 2023

robertbastian commented Jan 31, 2023

robertbastian commented Jan 31, 2023

sffc commented Jan 31, 2023

robertbastian commented Jan 31, 2023

sffc left a comment

robertbastian commented Feb 1, 2023

Adding all keys to testdata #3042

Adding all keys to testdata #3042

Conversation

robertbastian commented Jan 30, 2023 • edited Loading

Manishearth commented Jan 30, 2023

This comment was marked as spam.

sffc Jan 31, 2023

Choose a reason for hiding this comment

robertbastian Jan 31, 2023

Choose a reason for hiding this comment

sffc Jan 31, 2023

Choose a reason for hiding this comment

robertbastian Jan 31, 2023

Choose a reason for hiding this comment

robertbastian Jan 31, 2023

Choose a reason for hiding this comment

sffc commented Jan 31, 2023

robertbastian commented Jan 31, 2023 • edited Loading

sffc commented Jan 31, 2023

sffc commented Jan 31, 2023

robertbastian commented Jan 31, 2023

robertbastian commented Jan 31, 2023

sffc commented Jan 31, 2023

robertbastian commented Jan 31, 2023

sffc left a comment

Choose a reason for hiding this comment

robertbastian commented Feb 1, 2023

robertbastian commented Jan 30, 2023 •

edited

Loading

robertbastian commented Jan 31, 2023 •

edited

Loading