Repository navigation
Accept the specification's translation option names, and the behavior as text - #64
Draft
jwrosewell wants to merge 3 commits into
Draft
jwrosewell wants to merge 3 commits into
jwrosewell wants to merge 3 commits into
Conversation
…n one TranslationEngineBase and Translator stored the behavior they were given without checking it, and compared it with the MissingTranslationBehavior members. A configuration file can only give text, so a configuration naming "EmptyString" or "flow_error" matched no member and a missing translation kept its original value, with nothing to say the setting had been ignored. MissingTranslationBehavior.from_value now returns a member as it is, and turns text naming a member into that member. It takes the specification's names (Original, EmptyString, FlowError) and the members' own names and values, ignoring case and underscores, and raises ValueError for anything else. The engine and the translator use it, so an unknown value stops the engine being built. The parameter is still named behavior.
The specification names the Translation Engine's options SourceElementDataKey and MissingTranslationBehavior. Python already took source_element_data_key, but named the second option behavior. TranslationEngineBase and Translator now take missing_translation_behavior. behavior still works as a keyword-only deprecated alias that raises a DeprecationWarning, and when both are given missing_translation_behavior wins. Neither given means Original, as before. A value given by position is taken as missing_translation_behavior, as it was taken as behavior. The IP Intelligence translation engines now pass every option by its name, so they need a fiftyone_pipeline_translation that takes the new name. Their package requires fiftyone_pipeline_translation 4.5.43 or later, the version the release plan gives this commit.
The IP Intelligence translation engines pass the missing translation behavior by position. A value passed by position is taken as missing_translation_behavior by the new translation package and as behavior by the old one, so the engines work with either release and raise no deprecation warning without any change. Passing it by name needed a minimum version of fiftyone_pipeline_translation that cannot be known until the release that carries the new name, so the engines and their package's requirements are back to their rc content.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The Translation Engine's missing translation option differed from the specification in two ways. On rc at 0507eef, the problems were these.
SourceElementDataKey,FixedLanguageandMissingTranslationBehavior(translation-engine.md, "Configuration"). Python already tooksource_element_data_keyandfixed_language, the same names in Python's style. It named the third optionbehavior, both inTranslationEngineBase.__init__(translation_engine_base.py lines 62 to 68) and inTranslator.__init__(translator.py lines 41 to 44).MissingTranslationBehaviormembers and treats anything that is notEMPTY_STRINGorFLOW_ERRORasORIGINAL(translator.py lines 118 to 124).PipelineBuilder.build_from_configurationpasses each element'selementParametersto its constructor as they are, and a configuration file can only give text. So a configuration naming"EmptyString"or"FlowError", which are the specification's names for the behaviors (translation-engine.md, "Processing"), matched no member, and a missing translation kept its original value. A typing mistake was treated the same way.Change
All line numbers in this section are at d9ec5ee.
The specification's name, with the old name kept
TranslationEngineBaseandTranslatortakemissing_translation_behavior(translation_engine_base.py lines 62 to 71 and translator.py lines 42 to 47). A value given by position is taken asmissing_translation_behavior, as it was taken asbehaviorbefore.behaviorstill works as a keyword-only, deprecated alias. Using it raises aDeprecationWarningthat namesmissing_translation_behavior. When both are given,missing_translation_behaviorwins. When neither is given the behavior is Original, as before. This is done by_resolve(missing_translation_behavior.py lines 77 to 98), and both docstrings say so.source_element_data_keyandfixed_languagealready had the specification's names, so they are unchanged and have no alias.The behavior as text
MissingTranslationBehavior.from_value(missing_translation_behavior.py lines 43 to 69) returns a member as it is. It turns text into the member the text names, accepting the specification's names (Original,EmptyString,FlowError) and the members' own names and values (ORIGINAL,empty_string,FLOW_ERRORand so on), ignoring case and underscores.Noneincluded, raisesValueErrornaming the three accepted names. Because both constructors use it, an unknown value stops the engine, or a pipeline built from a configuration file, being built.The IP Intelligence translation engines
CountryCodeTranslationEngineandCountriesTranslationEngineare not changed. They pass the behavior by position, which the new constructor takes asmissing_translation_behaviorand the old one took asbehavior. So they work with either release offiftyone_pipeline_translation, raise no deprecation warning, andfiftyone_ipintelligence_translationneeds no new release for this change.No readme or example names the behavior option, so none changed.
For the release notes
missing_translation_behavior. The old name,behavior, still works but raises aDeprecationWarning, and if both are givenmissing_translation_behavioris used."EmptyString"or"FlowError", now takes effect, where before it was ignored and the original value was kept.ValueError.Tests
All the tests below are in
MissingTranslationBehaviorTestsinpipeline/fiftyone_pipeline_translation/tests/test_translation.py(line 226). They use a source holding "Germany", which has a French translation, and "Spain", which does not.The behavior as text
test_from_value_names_each_memberchecks twelve spellings, being each specification name, member name and member value, and other mixes of case and underscores.test_from_value_returns_a_member_as_it_ischecks each member.test_each_text_takes_effectbuilds an engine with each spelling and checks the result for "Spain". It stays "Spain" for Original, becomes an empty string for EmptyString, and becomesNonewith one flow error for FlowError.test_each_member_takes_effectdoes the same with each member.test_unknown_value_is_refused_when_the_engine_is_builtchecks that"","Empty String","Origin","Missing","flow-error",None,1and["Original"]each raiseValueErrornaming the accepted names.test_translator_takes_textbuilds aTranslatorwith"EmptyString".The option names
test_specification_name_takes_effect_without_a_warningpasses each specification name asmissing_translation_behavior, with deprecation warnings turned into errors.test_old_name_still_takes_effect_with_a_warningpasses each asbehavior, and checks for aDeprecationWarningnamingmissing_translation_behaviorand for the same results.test_specification_name_wins_when_both_are_givenpassesmissing_translation_behavior="EmptyString"andbehavior="FlowError", and checks for EmptyString's result and the warning.test_neither_name_given_is_originalandtest_none_given_by_either_name_is_refusedcheck that neither given means Original, and thatNoneis refused under either name.test_translator_takes_both_nameschecks the same three cases onTranslator.Configuration files
test_configuration_text_takes_effectbuilds the pipeline withbuild_from_configurationfrom a configuration that uses the specification's names,source_element_data_keyandmissing_translation_behavior. Every value is text, a list or a dictionary, as a configuration file gives them. It runs with"EmptyString","flow_error"and"Original", with deprecation warnings turned into errors.test_configuration_with_the_old_name_still_takes_effectusesbehaviorin the configuration and checks the warning and the result.test_configuration_with_an_unknown_value_is_refusedchecks that"EmptyStrings"stops the build.Verification
On Windows with Python 3.14.7, with
fiftyone_pipeline_core,fiftyone_pipeline_translationandfiftyone_ipintelligence_translationinstalled from this checkout into a virtual environment, I ranpython -m pytest tests -p no:cacheprovider -qin each package's folder.pipeline/fiftyone_pipeline_translation19 passed.47 failed, 28 passed, 6 subtests passed. The failures are the new tests. For example, a configuration naming"EmptyString"gave['Allemagne', 'Spain'], andmissing_translation_behaviorwas not a parameter.34 passed, 47 subtests passed, being the 19 existing tests and the 15 new ones.ip-intelligence/fiftyone_ipintelligence_translationThe IP Intelligence package's files are the same as on rc. I ran its tests with
-W error::DeprecationWarning, so a deprecation warning would fail them.fiftyone_pipeline_translationthey gave26 passed.fiftyone_pipeline_translationthey gave26 passed.Remaining
behavioralias would break callers, so it waits for a release where that is allowed.elementParameters, which the specification says stay in code. That is not changed here.