Adding source data versions to baked crates #4399

robertbastian · 2023-12-01T15:42:11Z

robertbastian · 2023-12-01T15:47:48Z

provider/core/src/datagen/mod.rs

-impl<T> ExportableProvider for T where
-    T: IterableDynamicDataProvider<ExportMarker> + DynamicDataProvider<AnyMarker> + Sync
-{
+    /// Returns the source versions of the data.


This breaks semver but I don't know how else I would get this data from the source provider. I really don't want to define a data key for this...

Adding the function to the trait with a default impl doesn't break semver, but removing the blanket impl does. Can you use runtime specialization in the blanket impl to sniff for the known concrete impl that has a source_versions function? Well I guess that requires making T: 'static which is also semver. I lean toward just deleting the blanket impl at least to get the PR building and then we can escalate to the team.

sffc · 2023-12-01T20:06:25Z

provider/datagen/src/provider.rs

+            tag.strip_prefix("release-")
+                .unwrap_or(tag)
+                .replace('-', ".")
+                .leak(),


Suggestion: often we use pre-release tags. Maybe just use the whole tag name?

The GH tags don't match the version in the ICU repo, so I have to process them somehow. release-75-rc will become 75.rc which I guess is fine. I do want release-75-1 to become 75.1, because that's the version that we would read from the files.

Why not just read it from the files?

I'm thinking about the tags like icu4x/2023-12-01/73.1 which I'd like to not need to parse. I'm okayish with bubbling them up to users, though I agree having an actual version number seems more useful.

Why not just read it from the files?

Because then 10 different tags will be 74.1.

How about we save the tag string in SourceVersions and then if we want we can add an instance method to SourceVersions that parses the ICU version out of the tag.

How about you let me parse release-xx-yy to xx.yy?

It seems like a more consistent mental model if SourceVersions::Public always contains the tag string.

If you want to parse the release version into SourceVersions::Public, you could make a new variant SourceVersions::Tagged for arbitrary tags that aren't release tags (including hashes, etc)?

sffc · 2023-12-01T20:07:44Z

provider/core/src/versions.rs

+/// A concrete version
+pub enum SourceVersion<'data> {
+    /// A released version
+    Public(#[cfg_attr(feature = "serde", serde(borrow))] &'data str),


Suggestion: use a Cow so we don't leak strings.

Can't have cows in consts because of dropping stuff.

sffc · 2023-12-01T20:09:34Z

provider/core/src/versions.rs

+pub struct SourceVersions<'data> {
+    #[cfg_attr(feature = "serde", serde(borrow))]
+    /// The version of CLDR source data, if used during datagen.
+    pub cldr: Option<SourceVersion<'data>>,


Observation/Issue: icu_provider has no concept of CLDR. I'd like to keep it agnostic if possible.

personally I think it's important to have this but it's not a strong opinion

sffc · 2023-12-01T20:19:48Z

provider/core/src/datagen/mod.rs

-impl<T> ExportableProvider for T where
-    T: IterableDynamicDataProvider<ExportMarker> + DynamicDataProvider<AnyMarker> + Sync
-{
+    /// Returns the source versions of the data.


Adding the function to the trait with a default impl doesn't break semver, but removing the blanket impl does. Can you use runtime specialization in the blanket impl to sniff for the known concrete impl that has a source_versions function? Well I guess that requires making T: 'static which is also semver. I lean toward just deleting the blanket impl at least to get the PR building and then we can escalate to the team.

sffc · 2023-12-01T20:20:23Z

Oh and thanks for this PR!

Manishearth · 2023-12-02T00:14:37Z

provider/core/src/versions.rs

+}
+
+#[cfg(feature = "datagen")]
+impl databake::Bake for SourceVersions<'_> {


thought: ideally would be very cool if e.g. the SourceVersions for properties did not mention a cldr version (since it's irrelevant)

but that seems to be more hassle than it's worth to track

sffc · 2023-12-07T18:48:47Z

@Manishearth - There are two pieces of information. One, what is the provenance of the data. Second, what is the versioning of that data source. So is it more useful to know your list of sources, or... ICU4C contains a lot of CLDR data and it could start exporting that data. What does that mean here? Do they reflect that as an ICU version or do they put it as the CLDR version?
@robertbastian - I see this as a feature for compiled data. If you are building your own data, you don't need this information. And baked data is always from our three datagen sources.
@younies - What's going on here... is it exposing the version of CLDR or LSTM?
@robertbastian - it's for everything, we want to expose the version of the source data. CLDR, icuexport, segmenter
@Manishearth - You need to know if you're up-to-date, etc.
@younies - Why not only in documentation?
@Manishearth - Because you want to access from code.
@sffc - @sven-oly's conformance suite wants to query the data version so it can select the correct test data.
@Manishearth - Chrome/V8 bubble this information up via version strings, bug reporting, ...
@sffc - This is just a fingerprint. Doesn't even need to be well structured.
@sffc - Don't like how it is in the global icu_provider crate. It could be a struct defined in each data crate.
@robertbastian - If this is metadata, why don't we put it in the data crates Cargo.toml? It's programatically accessible, and we're more flexible with the structure. I'm not a big fan of having a Rust API for this.
@younies - Structured data is better because you don't want to parse a string.
@sffc - we could put it in cargo.toml, would satisfy concern about schema. reading from cargo.toml is a pain. we do that for icu4x versions in the conformance test code.
@younies - don't like litemaps because you'll be forced to have strings and you have to parse them
@Manishearth - but we can use any type we want, including enums. if there are certain common structures we can add them to the enum
@robertbastian - We read the version numbers from either the git tag or... we don't get that guarantee ourselves.
@robertbastian - We can overdesign this but I really don't want to commit to a Rust API for this. For the conformance tests it's fine to put it in Cargo.toml
@Manishearth - want to highlight that conformance tests are an internal use case, we should make sure they have everything they need, but we shouldn't overindex on making it convenient unless other clients need the same thing.
@sffc - OK to do the Cargo.toml solution and relitigate this when we have a client other than the conformance suite.
@robertbastian - And the data crates can include a macro that generates docs with the version numbers.
@younies - How do people access the data from Cargo.toml?
@sffc - They have to read the Cargo.toml file. There is a cargo_metadata crate which makes it slightly easier.
@younies - How about a programmatic user?
@Manishearth - We can revisit that when we have a concrete use case other than @sven-oly's.

Consensus:

data crate Cargo.toml files contain an auto-generated metadata section with the versions:

[metadata.sources]
# some free structure chosen by datagen
cldr = { tagged = "44.0.0" }
icuexport = { custom = "74.0" }

data crates include the version number they use in their own doc string (e.g. in lib.rs)
Revisit an API solution if a client comes to us with a clear use case.

LGTM: @sffc @Manishearth @robertbastian @younies

sffc · 2023-12-07T23:27:18Z

provider/baked/_template_/Cargo.toml

@@ -15,3 +15,8 @@ homepage.workspace = true
 include.workspace = true
 repository.workspace = true
 rust-version.workspace = true
+
+[metadata.sources]


Nit: I think it should be [package.metadata.sources]

https://doc.rust-lang.org/cargo/reference/manifest.html#the-metadata-table

sffc · 2023-12-07T23:28:15Z

tools/bakeddata-scripts/src/main.rs

-                    .replace("_version_", version),
+                    .replace("_version_", version)
+                    .replace("_cldr_tag_", DatagenProvider::LATEST_TESTED_CLDR_TAG)
+                    .replace(
+                        "_icuexport_tag_",
+                        DatagenProvider::LATEST_TESTED_ICUEXPORT_TAG,
+                    )
+                    .replace(
+                        "_segmenter_lstm_tag_",
+                        DatagenProvider::LATEST_TESTED_SEGMENTER_LSTM_TAG,
+                    ),


Praise: This is, like... a super clean and simple way to do things. Good suggestion 👍

robertbastian requested review from sffc, Manishearth and a team as code owners December 1, 2023 15:42

robertbastian commented Dec 1, 2023

View reviewed changes

sffc reviewed Dec 1, 2023

View reviewed changes

Manishearth previously approved these changes Dec 2, 2023

View reviewed changes

robertbastian added the discuss-priority Discuss at the next ICU4X meeting label Dec 7, 2023

sffc removed the discuss-priority Discuss at the next ICU4X meeting label Dec 7, 2023

ok

f7bf8df

robertbastian dismissed Manishearth’s stale review via f7bf8df December 7, 2023 21:39

robertbastian force-pushed the sourceversionos branch from d9ca255 to f7bf8df Compare December 7, 2023 21:39

robertbastian requested review from sffc and Manishearth December 7, 2023 21:39

robertbastian changed the title ~~Piping source data versions through datagen~~ Adding source data versions to baked crates Dec 7, 2023

robertbastian added 3 commits December 7, 2023 22:43

fix

b453cce

ugh

ec096b7

ugh

4f603ed

sffc reviewed Dec 7, 2023

View reviewed changes

package

5bfaacd

robertbastian requested a review from sffc December 8, 2023 06:50

Manishearth approved these changes Dec 8, 2023

View reviewed changes

robertbastian merged commit 60b84ef into unicode-org:main Dec 8, 2023
29 checks passed

robertbastian deleted the sourceversionos branch January 4, 2024 14:45

robertbastian mentioned this pull request Feb 8, 2024

Add functions to get ICU4X, CLDR, and Unicode versions #4342

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding source data versions to baked crates #4399

Adding source data versions to baked crates #4399

robertbastian commented Dec 1, 2023 •

edited

Loading

robertbastian Dec 1, 2023

sffc Dec 1, 2023

sffc Dec 1, 2023

robertbastian Dec 1, 2023

sffc Dec 1, 2023

robertbastian Dec 1, 2023

sffc Dec 1, 2023

robertbastian Dec 1, 2023

sffc Dec 1, 2023

sffc Dec 1, 2023

robertbastian Dec 1, 2023

sffc Dec 1, 2023

Manishearth Dec 2, 2023

sffc Dec 1, 2023

sffc commented Dec 1, 2023

Manishearth Dec 2, 2023

sffc commented Dec 7, 2023

sffc Dec 7, 2023

sffc Dec 7, 2023

Adding source data versions to baked crates #4399

Adding source data versions to baked crates #4399

Conversation

robertbastian commented Dec 1, 2023 • edited Loading

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

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 Dec 1, 2023

Choose a reason for hiding this comment

sffc commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertbastian commented Dec 1, 2023 •

edited

Loading