Repository navigation
Use the specification's defaults for the performance profile and Update Matched User Agent - #57
Draft
jwrosewell wants to merge 4 commits into
Draft
jwrosewell wants to merge 4 commits into
jwrosewell wants to merge 4 commits into
Conversation
…s it The Device Detection on-premise specification says the option is on by default, so the UserAgents property holds the matched characters of the evidence. The C library sets it on, but the Python engine took update_matched_useragent=False and always passed it on, so UserAgents was a list of empty strings unless a caller turned the option on. The engine now takes None and only sets the option when a value is given, so the native default applies, as the .NET builder does. The docstrings in the engine and both builders say the option is on by default, and the match metrics example's comment no longer says it has to be set. A new test builds a pipeline on the free Lite data file with the option left unset, set on and set off, and skips when that file is not there.
The Device Detection on-premise specification gives Balanced as the default performance profile, and the C library's default configuration is the Balanced one, but the Python engine defaulted to LowMemory. The engine now defaults to "Balanced", which its profile switch already maps to the native Balanced configuration, and the docstrings in the engine and both builders say so. Balanced loads some collections into memory and caches others, where LowMemory reads everything from the data file, so a caller relying on the default uses more memory. Naming "LowMemory" keeps the previous behaviour. A new test compares the collection settings of the configuration an engine built at its defaults gives the native code with those of each profile, because the engine keeps no name for the profile in use.
MaxPerformance and LowMemory have the same collection settings, as do Balanced and BalancedTemp, so the test's comparison of collection settings cannot separate either pair. The test's docstring now says so.
The max_matched_useragent_length docstrings in the engine and both builders said the option is ignored when update_matched_useragent is false, and named it max_matched_useragents_length. The C library's header (src/dd/config-dd.h) says the limit applies to a single header, a pseudo header and the matched User-Agent, that a longer value is ignored and detection moves on, and that the limit applies whether or not the matched User-Agent is recorded. The docstrings now say the same, give the native default of 500, and spell the option's name correctly.
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 Device Detection on-premise engine in Python used two defaults that differ from the specification and from the C library it is built on.
fiftyoneDegreesHashDefaultConfig = { FIFTYONE_DEGREES_HASH_CONFIG_BALANCED }(51degrees-cxxsrc/dd/hash/hash.clines 356 to 379, at the commit thecxxsubmodule points to), andConfigHash()starts from it (ConfigHash.cpp lines 28 to 32). Python defaulted to "LowMemory".UserAgentsmatch metric holds the characters of the evidence that were matched unless the option is turned off (the UserAgents row at line 273 and the Update Matched User Agent row at line 289, from specifications#95), with the default "true, from native code". The C library sets it on,#define FIFTYONE_DEGREES_CONFIG_DEVICE_DETECTION_UPDATE_DEFAULT true(src/dd/config-dd.hline 82). Python turned it off.The .NET builder leaves the native default alone for Update Matched User Agent unless
SetUpdateMatchedUserAgentis called, and documents it as"true - " + NATIVE_DEFAULTS(DeviceDetectionHashEngineBuilderBase.cs lines 187 to 201).What I checked on rc
All line numbers in this section are on rc at 0507eef, in
device-detection/fiftyone_devicedetection_onpremise/src/fiftyone_devicedetection_onpremise/devicedetection_onpremise.pyunless another file is named.DeviceDetectionOnPremise.__init__tookupdate_matched_useragent=False(line 64) andperformance_profile="LowMemory"(line 65).config.setBalanced(). In the C++ wrappersetBalancedcopies the collection settings offiftyoneDegreesHashBalancedConfig(ConfigHash.cpp lines 58 to 60), so no new mapping was needed.config.setUpdateMatchedUserAgent(update_matched_useragent)(line 212), so it turned the native default off.DeviceDetectionOnPremisePipelineBuilder,DeviceDetectionPipelineBuilderandPipelineBuilder.build_from_configurationhold no default of their own for either option. They pass their settings to that constructor, so every route, a configuration file'selementParametersincluded, took the engine's defaults. A configuration file can set either option by name, asperformance_profileandupdate_matched_useragent.ResultsHash::getUserAgentthen answers with an empty string (ResultsHash.cpp lines 280 to 291). SoUserAgentswas a list of empty strings, one per result. The existingtest_matched_user_agentsonly checks the list has at least one entry, which an empty string satisfies, so it passed either way.max_matched_useragent_lengthis ignored whenupdate_matched_useragentis false, and named itmax_matched_useragents_length. The C header says the limit applies whether or not the matched User-Agent is recorded (config-dd.h lines 57 to 68), and its default is 500 (line 106). The engine only sets the limit when a value is given.Change
DeviceDetectionOnPremisetakesperformance_profile="Balanced"(line 65).update_matched_useragent=None(line 64) and only callssetUpdateMatchedUserAgentwhen a value is given (lines 225 to 227), so the native default applies otherwise, as in .NET.devicedetection_onpremise.py,devicedetection_onpremise_pipelinebuilder.pyandfiftyone_devicedetection/devicedetection_pipelinebuilder.pygive the defaults. Balanced is the default profile, as in the native code. Update Matched User Agent is a bool, fillsUserAgents, is on by default, andFalsesaves recording the characters whenUserAgentsis not needed.max_matched_useragent_lengthdocstrings in the same three files now spell the option's name correctly and say what the C header says. The limit applies to a single header such as User-Agent, to a pseudo header including the separators between its values, and to the matched User-Agent. A longer value is ignored and detection moves on to the next header. The limit applies whether or notupdate_matched_useragentis on, the native default of 500 applies when it is left unset, and a larger limit raises the memory each set of results holds. No code changes for this option.True, no longer says the example has to set it.A caller that names a profile, or passes
update_matched_useragent=False, keeps what they asked for. Only a caller that left either option unset sees a difference.For the release notes
performance_profile="LowMemory"to keep the previous behaviour.UserAgentsholds the matched characters of the evidence. Passupdate_matched_useragent=Falseto turn it off.Tests
Both new files are in
device-detection/fiftyone_devicedetection_onpremise/tests. Each finds the free Lite data file atcxx/data/device-detection/51Degrees-LiteV4.1.hashfrom its own folder, wherecxx/data/get-data-files.ps1andget-data-files.shdownload it and wheredevice-detection/ci/fetch-assets.ps1links it in, and skips with a message naming the download scripts when the file is not there.test_performance_profile_default.py. This test is indirect. Python keeps no name for the profile in use, so the tests compare the configuration the engine gives the native code (engine.config) with the settings the native code gives each profile. They compare whether the strings, properties, values, profiles, nodes and profile offsets collections are loaded into memory, and how many items each caches, which are what a profile sets and what Balanced and LowMemory differ in. MaxPerformance and LowMemory have the same collection settings, as do Balanced and BalancedTemp, so the test cannot tell either pair apart.test_balanced_and_low_memory_differchecks the two profiles' settings differ, so the comparisons below can tell them apart.test_engine_defaults_use_balancedbuilds the engine at its defaults in a pipeline and checks its settings are the Balanced ones.test_builder_defaults_use_balanceddoes the same throughDeviceDetectionOnPremisePipelineBuilder.test_each_named_profile_is_usedchecks each of the five names gives the settings its own setter gives.test_update_matched_useragent.pybuilds a pipeline withDeviceDetectionOnPremisePipelineBuilder, processes one User-Agent and readsUserAgents.test_builder_defaults_populate_user_agentsleaves the option unset, and checks that at least one character was matched and that every matched character is the evidence's character at the same position.test_turned_on_populates_user_agentsdoes the same with the option set toTrue.test_turned_off_leaves_user_agents_emptysets it toFalseand checks every entry is an empty string.Verification
From
device-detection/fiftyone_devicedetection_onpremise, on Windows with Python 3.14.7 and the Visual Studio 2022 Build Tools. I installed the pipeline and Device Detection packages from this checkout into a virtual environment, building the native module withpip install -e ., and downloaded the Lite file and20000 User Agents.csvwithcxx/data/get-data-files.ps1. Then I ran--ignore) gave57 passed.3 failed, 4 passed, 5 subtests passed. The three failures are the three defaults tests.test_engine_defaults_use_balancedandtest_builder_defaults_use_balancedfound the LowMemory settings, being nothing loaded and no cache, where Balanced caches 10000 strings and 50000 nodes and loads the properties.test_builder_defaults_populate_user_agentsfound['', '', '', ''].64 passed, 5 subtests passed, being the 57 existing tests and the 7 new ones.At the builder's defaults on this branch, the first of the four matched User-Agents for the test's iPhone User-Agent reads
______________Phone;______________ 11_2_______________________________________________________ Mobile_______, where_marks a character that was not matched.I did not run the Device Detection cloud tests or the examples' tests locally.
Remaining
fiftyone_devicedetection_cloud/devicedetection_cloud_pipelinebuilder.pystill carries a copy of the on-premise settings in its docstring, including the old wording formax_matched_useragent_length. The cloud builder does not use those settings, so I have left that copy alone.