Repository navigation
Use the Balanced performance profile by default for IP Intelligence - #62
Draft
jwrosewell wants to merge 1 commit into
Draft
jwrosewell wants to merge 1 commit into
jwrosewell wants to merge 1 commit into
Conversation
The IP Intelligence on-premise specification gives Balanced as the default performance profile, and the C library's default IP Intelligence 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, on the free ASN data file, 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.
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 IP Intelligence on-premise specification gives "Balanced" as the default performance profile (ip-intelligence-on-premise.md, the Performance Profile row at line 259). The C library's default IP Intelligence configuration is the Balanced one,
fiftyoneDegreesIpiDefaultConfig = { FIFTYONE_DEGREES_IPI_CONFIG_BALANCED }(51degrees-cxxsrc/ipi/ipi.clines 237 to 257, at the commit thecxxsubmodule points to). Python's IP Intelligence engine defaulted to "LowMemory".The Device Detection engine gets the same change in #57. This pull request is separate because it is a different package.
What I checked on rc
All line numbers in this section are on rc at 0507eef, in
ip-intelligence/fiftyone_ipintelligence_onpremise/src/fiftyone_ipintelligence_onpremise/ipintelligence_onpremise.py.IpIntelligenceOnPremise.__init__tookperformance_profile="LowMemory"(line 60).ConfigIpiSwig()and always calls one profile setter. It maps "Balanced" toconfig.setBalanced(), which in the C++ wrapper copiesfiftyoneDegreesIpiBalancedConfig(ConfigIpi.cpp lines 53 to 55). So no new mapping was needed.IpIntelligenceOnPremisePipelineBuilder,IpIntelligencePipelineBuilderandPipelineBuilder.build_from_configurationhold no default of their own. They pass their settings to that constructor, so every route, a configuration file'selementParametersincluded, took the engine's default. A configuration file can set the profile by name, asperformance_profile.Change
IpIntelligenceOnPremisetakesperformance_profile="Balanced"(line 60).performance_profiledocstrings inipintelligence_onpremise.py,ipintelligence_onpremise_pipelinebuilder.pyandfiftyone_ipintelligence/ipintelligence_pipelinebuilder.pysay Balanced is the default, as in the native code.A caller that names a profile keeps what they asked for. Only a caller that left the profile unset sees a difference. Concurrency is not changed.
For the release notes
The IP Intelligence on-premise engine now uses the Balanced performance profile by default, instead of LowMemory, as the specification and the C library give. Anyone relying on the default will use more memory after upgrading, because Balanced loads some collections into memory and caches others, where LowMemory reads everything from the data file. Pass
performance_profile="LowMemory"to keep the previous behaviour.Tests
ip-intelligence/fiftyone_ipintelligence_onpremise/tests/test_performance_profile_default.pyuses the free ASN data file atcxx/data/ip-intelligence/51Degrees-IPIV4AsnIpiV41.ipi, through theget_asn_data_file_pathhelper the existing tests use. It skips with a message naming the download scripts when that file is not there.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, for seven collections (strings, maps, graphs, profile groups, profile offsets, property types and the graph), whether each is loaded into memory and how many items it caches. Those are what a profile sets, and Balanced and LowMemory differ in all seven. MaxPerformance and LowMemory have the same collection settings, as do Balanced and BalancedTemp, so the test cannot tell either pair apart.The other four collections the configuration exposes, being components, properties, values and profiles, are left out, because asking for them corrupts the heap. In the committed wrapper
ipi_python_wrap.cxx,ConfigIpiSwig.getComponents,getProperties,getValuesandgetProfilesreturn a reference to a setting the configuration owns, but mark itSWIG_POINTER_OWN, so Python frees memory it does not own. The other seven getters do not. The cause is in the C library's SWIG files, where%newobject getComponents,getProperties,getProfilesandgetValuesare written without a class name (src/common/MetaData.ilines 41 to 44), so they also catch the getters of the same names insrc/ipi/ConfigIpi.i, which return references. My first run of this test called them and stopped with Windows fatal exception 0xc0000374, which is heap corruption. The Device Detection wrapper returns its collection settings by value, so it is not affected.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 throughIpIntelligenceOnPremisePipelineBuilder.test_each_named_profile_is_usedchecks each of the five names gives the settings its own setter gives.Verification
From
ip-intelligence/fiftyone_ipintelligence_onpremise, on Windows with Python 3.14.7 and the Visual Studio 2022 Build Tools. I installed the pipeline packages andfiftyone_ipintelligence_sharedfrom this checkout into a virtual environment, built this package's native module withpip install -e ., and downloaded the ASN file and the free Lite file withcxx/data/get-data-files.ps1. The existing tests read the Lite file throughIPINTELLIGENCEDATAFILE, set tocxx/data/ip-intelligence/51Degrees-LiteV41.ipi. Then I ran--ignore) gave16 passed.2 failed, 2 passed, 5 subtests passed. The failures aretest_engine_defaults_use_balancedandtest_builder_defaults_use_balanced, which found the LowMemory settings, being nothing loaded and no cache.20 passed, 5 subtests passed, being the 16 existing tests and the 4 new ones, with the existing tests now running on the Balanced profile.I did not run the IP Intelligence cloud, translation or examples tests locally, and CI does not run the build and test on a draft.
Remaining