FIX: Run cloud examples without the on-premise engine, report why a property has no value, and make a broken example go red - #344
Merged
Conversation
… a property has no value, and make a broken example go red Four of the eight cloud examples imported fiftyone_devicedetection.devicedetection_pipelinebuilder, which unconditionally imports the on-premise engine, so a cloud only example could not be run without building the on-premise native library from source. PyPI publishes that package as an sdist with no wheel, so anyone following a cloud example needed a C++ toolchain first. The four now use DeviceDetectionCloudPipelineBuilder, which does exactly the same work on the cloud path, and one of them imported the builder without ever using it. A new test imports every cloud example in a separate process with the on-premise packages blocked, so the dependency cannot come back. The hardware profile cloud engine threw away the reason the cloud service gives when a resource key is not entitled to a property, so the TAC and native model examples stopped with AttributeError: 'NoneType' object has no attribute 'no_value_message'. The engine now carries each aspect level value and its companion nullreason into every profile, and get_human_readable reports the reason instead of calling a method on None. A new unit test runs the engine against fixed cloud responses, so it needs no resource key and no network connection, and the cloud package is added to the unit job so it runs on every build. The cloud example tests asserted nothing about what the examples printed. They now capture the output and assert the headings each example must print, that nothing in it reads as a programming fault, and that each lookup says something about the devices it found. Tests that cannot run without a resource key now skip with a message naming the variable they wanted, rather than returning quietly and passing. The property coverage tests skip when the CSV asset is absent rather than failing to import. Resource key environment variables move onto the '_51DEGREES_RESOURCE_KEY' convention, with the previous names still read as a fallback so an existing setup keeps working.
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.
Why
Three faults, found by installing only the cloud packages and then running
the cloud examples against a resource key that has the hardware aspect but
is not entitled to the hardware properties themselves.
1. Half the cloud examples could not run at all. Four of the eight
imported
fiftyone_devicedetection.devicedetection_pipelinebuilder, whichimports
fiftyone_devicedetection_onpremiseunconditionally, so acloud only example needed the on-premise native engine. PyPI publishes
that package as an sdist with no wheel, so anyone following a cloud example
had to build C++ from source first:
2. A missing entitlement was handled badly. The cloud service answers
correctly and says why it has no value, for example
"hardwarevendornullreason": "HardwareVendor is a paid feature. You need a licence key to retrieve data.", butHardwareProfileCloudread onlycloud_data["hardware"]["profiles"]and threw the reasons away. Eachprofile therefore had no
hardwarevendorat all, soExampleUtils.get_human_readablecalledno_value_message()onNone.3. The tests could not report either fault. The cloud example tests ran
the examples but asserted nothing about what they printed, and the cloud
package's own tests were not in the unit job at all. Tests that could not
find a resource key returned quietly and passed, so a run that proved
nothing looked the same on the dashboard as a run that proved something.
What changed
The examples run with only the cloud packages
configurator_console.py,metadata_console.pyandgettingstarted_web/app.pyuseDeviceDetectionCloudPipelineBuilder,which does exactly the same work on the cloud path.
DeviceDetectionPipelineBuilderdiffers only in that it adds theon-premise engine and usage sharing when
data_file_pathis given, whicha cloud example never gives.
gettingstarted_console.pyimported the builder and never used it,because it builds from the configuration file. The import is gone.
tests/test_cloudexampleimports.pyimports every cloud examplein a separate process with
fiftyone_devicedetection_onpremiseandfiftyone_devicedetectionblocked, so the dependency cannot come backunnoticed. It needs no resource key and no network connection.
This is a change in the examples, not in the
fiftyone_devicedetectionpackage. That package is a deliberate wrapper that offers both engines
through one builder, so making its import of the on-premise engine lazy
would be a separate decision about the package's contract. The examples
should not have been reaching for the wrapper in the first place.
The reason a property has no value is reported
hardwareprofile_cloud.pybuilds the aspect level values from the cloudresponse, pairing each null value with its companion
<name>nullreason,and adds them to every profile that does not already carry that property.
A response with no
hardwaresection no longer raises.ExampleUtils.get_human_readabledistinguishes three cases, being avalue, no value with a reason, and a property that is not in the results
at all, and names the property in the last case.
ExampleUtils.get_profilesandget_no_profiles_messagelet the TAC andnative model examples say plainly that no profiles came back.
ismobilethrough the helper, so aresource key without that property reports the reason rather than
stopping part way through.
The tests can now fail
fiftyone_devicedetection_cloud/tests/test_hardwareprofile_cloud.pyruns the engine against fixed cloud responses, so it needs no resource
key and no network connection.
ci/run-unit-tests.ps1runs the cloud package as well as the on-premiseone, so that test runs on every build rather than only when the
integration job has a key.
tests/test_cloudexamples.pycaptures what each example writes andasserts the headings it must print, that nothing in the output reads as a
programming fault, and that each lookup says something about the devices
it found.
fiftyone_devicedetection_cloud/tests/test_cloud.pyskips with a messagenaming the variable it wanted, rather than returning quietly.
test_properties.pyskips at module level instead of raising, and alsoskips when the
51Degrees.csvasset is absent, so a missing asset is notreported as a failure.
_51DEGREES_RESOURCE_KEYconvention, in
example_utils.py, the cloud tests, both tox files andci/run-integration-tests.ps1. The previous name is still read as afallback so an existing setup keeps working.
Before
Installed packages were
fiftyone_devicedetection_cloud,fiftyone_devicedetection_sharedandfiftyone_devicedetection_examples,with no on-premise package, on Python 3.14.5:
With the on-premise package in place, and a resource key that has the
hardware aspect but no entitlement to the hardware properties:
After
Same three packages, still no on-premise package:
Same key, same command:
The tests now go red
The new import test, against the example code as it was before this change:
The new unit test, against the engine as it was before this change:
The rewritten example tests, against the engine and helper as they were
before this change, with the not entitled key:
Against this branch, same key:
And with no resource key set at all, which is what the unit job will do:
Each skip names the variable it wanted:
One thing for a reviewer to decide
The organisation secret still has its old name. The workflows now read
secrets._51DEGREES_RESOURCE_KEY_SUPERand fall back tosecrets.SUPER_RESOURCE_KEY, so nothing breaks either way. Renaming theorganisation secret would let common-ci's central
steps/set-resource-keys.ps1export it, after which both fallbacks can go. That rename needs
organisation access and is not done here.
Produced with AI assistance. Every claim above was checked by running the
commands shown, and the output is real rather than illustrative. It needs
human review before merging.