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

with_locales_no_fallback does fallback #4879

Open
robertbastian opened this issue May 8, 2024 · 14 comments
Open

with_locales_no_fallback does fallback #4879

robertbastian opened this issue May 8, 2024 · 14 comments
Labels
question Unresolved questions; type unclear

Comments

@robertbastian
Copy link
Member

The following snippet panics because HelloWorldProvider doesn't have fallback data

DatagenDriver::new()
    .with_keys([icu_provider::hello_world::HelloWorldV1Marker::KEY])
    .with_locales_no_fallback([langid!("de")], Default::default())
    .export(
        &icu_provider::hello_world::HelloWorldProvider,
        icu_datagen::blob_exporter::BlobExporter::new_with_sink(Box::new(std::io::sink())),
    )
    .unwrap();
@robertbastian robertbastian added the T-bug Type: Bad behavior, security, privacy label May 8, 2024
@robertbastian
Copy link
Member Author

Context: I'm trying to replace https://github.com/robertbastian/icu4x/blob/ec32ad4a79707b1ddb7852ed1653ac2d86c27f3f/provider/datagen/src/baked_exporter.rs#L25 by a version that doesn't use the network (the current test does). However I cannot figure out an invocation for baked that doesn't use fallback.

@sffc
Copy link
Member

sffc commented May 8, 2024

_no_fallback only means no runtime fallback. It still does datagen-time fallback.

You can avoid the datagen-time fallback by using LocaleFamily::FULL. But, hmm, it's not possible to pass a LocaleFamily into .with_locales_no_fallback.

@sffc
Copy link
Member

sffc commented May 8, 2024

The invocation I've used in a number of other places to avoid this problem is

.with_locales_and_fallback([LocaleFamily::FULL], Default::default())

@sffc sffc removed their assignment May 8, 2024
@sffc sffc added question Unresolved questions; type unclear and removed T-bug Type: Bad behavior, security, privacy labels May 8, 2024
@sffc
Copy link
Member

sffc commented May 8, 2024

It's not a bug, so unassigning myself.

@sffc
Copy link
Member

sffc commented May 8, 2024

The function docs say: "Sets this driver to generate the given locales assuming no runtime fallback."

It's totally reasonable that the driver needs datagen-time fallback in order to generate data that requires no runtime fallback. That's the only way to ensure that the input list of langids can all be fully resolved at datagen time.

We may have previously avoided loading the fallbacker if the explicit langid had an exact match with the supported locales, but now we always load the fallbacker in order to pick up any aux keys and locale extensions from the parent locales, which was a bug before.

@robertbastian robertbastian removed their assignment May 8, 2024
@robertbastian
Copy link
Member Author

This invocation works with HelloWorldProvider:

//! DatagenDriver::new()
//! .with_keys([icu_provider::hello_world::HelloWorldV1Marker::KEY])
//! .with_all_locales()
//! .export(&icu_provider::hello_world::HelloWorldProvider, exporter)
//! .unwrap();

It uses implicit exporter-derived defaults - what's the equivalent invocation for a baked provider?

@robertbastian
Copy link
Member Author

I guess it's .with_all_locales().with_fallback_mode(FallbackMode::Hybrid), which doesn't exist on the new API?

@sffc
Copy link
Member

sffc commented May 8, 2024

.with_all_locales().with_fallback_mode(FallbackMode::Hybrid)

is the same as

.with_locales_and_fallback([LocaleFamily::FULL], FallbackOptions {
    deduplication_strategy: Some(DeduplicationStrategy::None),
    runtime_fallback_location: Some(RuntimeFallbackLocation::External),
})

@sffc
Copy link
Member

sffc commented May 8, 2024

This transformation is implemented in code

            (_, Some(legacy_locales), FallbackMode::Hybrid) => {
                LocalesWithOrWithoutFallback::WithFallback {
                    families: map_legacy_locales_to_locales_with_expansion(legacy_locales),
                    options: FallbackOptions {
                        runtime_fallback_location: Some(RuntimeFallbackLocation::External),
                        deduplication_strategy: Some(DeduplicationStrategy::None),
                    },
                }
            }

@sffc
Copy link
Member

sffc commented May 8, 2024

  • [LocaleFamily::FULL] disables locale selection fallback (it forwards all supported locales to the output)
  • DeduplicationStrategy::None disables the deduplicator, which requires fallback
  • RuntimeFallbackLocation::External disables runtime fallback and asserts that LocaleFallbackProvider will be supplied at runtime, so the exporter can proceed with that assumption

I think you should be good if you set all three of those.

@robertbastian
Copy link
Member Author

So what's the difference between with_locales_and_fallback with ::None and ::External, and with_locales_no_fallback?

@sffc
Copy link
Member

sffc commented May 8, 2024

So what's the difference between with_locales_and_fallback with ::None and ::External, and with_locales_no_fallback?

with_locales_no_fallback is preresolved...

It might be mechanically the same as with_locales_and_fallback with ::None and ::External if all of the locale families are ::single. However, with_locales_and_fallback carries with it the assertion that a LocaleFallbackAdapter will be provided at runtime.

@sffc sffc added the discuss-priority Discuss at the next ICU4X meeting label May 15, 2024
@sffc
Copy link
Member

sffc commented May 15, 2024

Does this issue change your position on flattening these options to DatagenDriver?

impl DatagenDriver {
  pub fn with_locale_families(...)
  pub fn with_deduplication(...)
  pub fn with_runtime_fallback_location(...)
}

And perhaps change RuntimeFallbackLocation::External to RuntimeFallbackLocation::ExternalOrDisabled. Hmm.

@robertbastian
Copy link
Member Author

I think I'm fine flattening it.

@robertbastian robertbastian removed the discuss-priority Discuss at the next ICU4X meeting label May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Unresolved questions; type unclear
Projects
None yet
Development

No branches or pull requests

2 participants