Skip to content

Make the example profile comments and the Lite file name match the code - #65

Draft
jwrosewell wants to merge 1 commit into
rcfrom
fix/examples-profile-text
Draft

jwrosewell wants to merge 1 commit into
rcfrom
fix/examples-profile-text

Conversation

@jwrosewell

Copy link
Copy Markdown
Contributor

Problem

Four places in the examples and readmes say something about performance profiles or data files that the code does not do. All line numbers in this section are on rc at 0507eef.

  1. The IP Intelligence data file update example takes the engine's default profile for the Enterprise data file. In datafileupdate_console.py lines 185 to 203, the pipeline that downloads and reloads the Enterprise file sets no performance_profile, so it uses the engine's default. That is LowMemory on rc, and #62 changes the default to Balanced, which would hold more of the large Enterprise file in memory. The same example's other pipeline, which reads the free ASN file when there is no licence key, already names LowMemory (line 118).
  2. The Device Detection getting started example's comment names the wrong profile. In gettingstarted_console.py lines 68 to 74 the comment says "We use the low memory profile" and the code sets performance_profile = "MaxPerformance".
  3. The IP Intelligence performance example's header describes LowMemory as if it were the default. performance_console.py lines 36 to 40 says that with LowMemory the "memory mapped data file is shared between the processes". The example's default is MaxPerformance (run at line 133 and the -p option at line 217), so the sharing only happens when -p LowMemory is given. The C library also does not memory map the file, as no source under cxx/src/common or cxx/src/ipi calls mmap, MapViewOfFile or CreateFileMapping.
  4. The IP Intelligence package readme names the Lite file wrongly. fiftyone_ipintelligence/readme.md line 28 gives 51Degrees-LiteIpiV41.ipi. The data repository's download scripts save the file as 51Degrees-LiteV41.ipi, from the link cxx/data/ip-intelligence/51Degrees-LiteV41.ipi.link, and the data repository's readme lists it under that name. The examples' constant LITE_DATAFILE_NAME and the on-premise package's readme already use 51Degrees-LiteV41.ipi.

51Degrees-LiteIpiV41.ipi is the name of CI's copy of the same file. ip-intelligence/ci/fetch-assets.ps1 fetches the CI asset under that name and links it into cxx/data/ip-intelligence/51Degrees-LiteV41.ipi, and the on-premise unit tests read the CI copy at assets/51Degrees-LiteIpiV41.ipi (fiftyone_ipintelligence_onpremise/tests/constants.py line 43). So the line in ip-intelligence/readme.md that says the unit tests default to assets/51Degrees-LiteIpiV41.ipi is correct and is not changed.

Change

  1. datafileupdate_console.py sets performance_profile = "LowMemory" on the update pipeline, with a comment saying the Enterprise file is large so LowMemory reads it from disk. The example now keeps LowMemory whatever the engine's default is.
  2. gettingstarted_console.py says it uses MaxPerformance, which loads the whole data file into memory, and that this costs little memory because the free Lite file is about 35 MB, and gives the fastest detections. The profile itself is not changed.
  3. performance_console.py says that with the default MaxPerformance each worker loads the whole data file into its own memory, and that with -p LowMemory each worker reads the file from disk, so the operating system's file cache holds one copy for all the workers.
  4. fiftyone_ipintelligence/readme.md names the Lite file 51Degrees-LiteV41.ipi.

Lite file examples and every other example keep the profile they name.

Verification

From the repository root, on Windows with Python 3.14.7, I byte-compiled the three changed examples with python -m py_compile, which passed.

I then built the IP Intelligence packages from this checkout into a virtual environment, with the native module built locally by pip install -e . and the free ASN and Lite data files downloaded by cxx/data/get-data-files.ps1, and ran the example tests that import the two changed IP Intelligence examples. From ip-intelligence/fiftyone_ipintelligence_examples

python -m pytest tests/test_examples_without_settings.py tests/test_examples.py -k "datafileupdate or performance"

gave 3 passed, 17 deselected, being test_datafileupdate_console, test_performance_console and test_onpremise_performance_console. The whole of tests/test_examples_without_settings.py gave 7 passed, 1 skipped, the skip being the mixed example, which needs Device Detection installed.

The update pipeline in datafileupdate_console.py only runs with an Enterprise licence key, which the tests do not have, so the tests cover the example's path without a key and not the changed line. The Device Detection getting started change is a comment, and I did not build Device Detection for it.

Remaining

  • #62 changes the IP Intelligence engine's default to Balanced. This pull request does not depend on it, and keeps the data file update example on LowMemory either way.

- The IP Intelligence data file update example now names the LowMemory
  profile for the pipeline that downloads and reloads the Enterprise data
  file, rather than taking the engine's default.
- The Device Detection getting started example's comment said it uses the
  low memory profile, but the code sets MaxPerformance. The comment now
  says MaxPerformance, and why it suits the free Lite data file.
- The IP Intelligence performance example's header said worker processes
  share a memory mapped data file under LowMemory, as if that were the
  default. It now says the default MaxPerformance loads the whole file
  into each worker, and that -p LowMemory lets the workers share the
  operating system's file cache.
- The IP Intelligence package readme named the Lite data file
  51Degrees-LiteIpiV41.ipi, which is CI's name for it. The data
  repository's scripts save it as 51Degrees-LiteV41.ipi.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant