Skip to content

Log the warnings of every cloud response, and a missing product as a warning - #123

Merged
jwrosewell merged 2 commits into
rcfrom
fix/cloud-warnings
Nov 14, 2026
Merged

jwrosewell merged 2 commits into
rcfrom
fix/cloud-warnings

Conversation

@jwrosewell

@jwrosewell jwrosewell commented Nov 14, 2026 •

Copy link
Copy Markdown
Contributor

Problem

  1. The warnings a cloud response carries are never logged. The cloud service can answer with a warnings array beside its results, and no code in fiftyone.pipeline.cloudrequestengine reads it. The request engine parses the discovery response for its products (cloudRequestEngine.js lines 404 to 415 at 6d5d4fef) and keeps the processing response as it is (lines 504 to 515), so a warning from the service reaches nobody.
  2. A product the key does not include is logged as an error. The cloud engine logs it once, at error level (cloudEngine.js lines 131 to 141), though nothing is wrong with the request and the other products in the answer are used.

What the specification says

From pipeline-specification/pipeline-elements/cloud-request-engine.md lines 440 and 441 at 706de1f0:

When making requests to the remote server during start-up or processing, some common steps MUST be followed.

From lines 460 and 461 of the same page, one of those steps:

Similarly to the errors array, any entries in the warnings array in the response MUST be logged as warnings.

From pipeline-specification/pipeline-elements/cloud-aspect-engine.md lines 63 to 69 at 706de1f0:

Once the Cloud Request Engine's discovery has completed, a Resource Key can be valid and still include no product that holds a particular Cloud Aspect Engine's Properties. That is not a definitive configuration error, so the Cloud Aspect Engine MUST NOT fail the Pipeline build. It builds with empty Property metadata and MUST log the missing product once per Cloud Aspect Engine instance, at warning level, naming its Data Key. It MUST NOT log the missing product again on each response.

.NET logs the response's warnings (CloudRequestEngine.cs lines 798 to 807 at f0ba9653), masking the licence key in each as in the errors.

What changed

  1. A new CloudRequestEngine.logWarnings logs each entry of a response's warnings array at warn level, with the licence key masked as it is in the errors, because the service's text can name the key it was sent. The discovery response's warnings are logged to every Pipeline the engine is in, and a processing response's to the Pipeline of that request. The response is otherwise used as before, and a response that is not JSON or has no warnings array logs nothing.
  2. The cloud engine logs a missing product at warn level, still once for each engine and naming its data key. A null product, which is an answer to that request, is still an error each time.
  3. types/cloudRequestEngine.d.ts declares logWarnings, written in the form npm run tsc writes, and a comment in pipeline.js no longer says a missing element is logged as an error.

Before you upgrade

An application that watches its log for errors no longer gets one when the resource key includes no product that a cloud engine in its Pipeline reads. That is now logged at warning level, once for each engine. Warnings the cloud service sends with a response are now logged at warning level, where they were not logged at all.

Tests

  • The new pipeline/fiftyone.pipeline.cloudrequestengine/tests/cloudWarnings.test.js checks that each warning in a processing response and in the discovery response is logged as a warning with no error, that a licence key a warning names is masked, and that a response with no warnings logs none.
  • In pipeline/fiftyone.pipeline.cloudrequestengine/tests/cloudEngineMissingElement.test.js, the missing product is now read from the warnings, once over two requests, naming the data key, with no error, under a resource key and under a licence key alone.

Red, run 43622252184 on d939f5b, the test commit on rc's code, with only a throwaway workflow added. On Ubuntu and on Windows 5 tests fail and the other 620 pass. Three are the new warning tests, "each warning in a response to a request is logged as a warning", "each warning in the discovery response is logged as a warning" and "a license key a warning names is masked", and two are the missing product tests that now look for a warning, "an answer without the element resolves and warns the key lacks it" and "a missing element names requestedProperties as a cause" under a licence key alone.

Green, run 43622265112 on 900fead, this head, with only the same throwaway workflow added. All 625 pass on Ubuntu and on Windows.

Types, run 43622274931 ran npm run tsc on this head merged with the head of #113, which writes the types again, and found no difference in the committed types but pipeline/fiftyone.pipeline.did/types/owid-js/v1.d.ts, which tsc writes for the OWID submodule already on rc and which #113 leaves to a later change. So the declaration of logWarnings written here by hand is what tsc writes.

Full build

Run 43637563411 on tmp/pr-matrix-123, started by tools/pr-matrix.py without macOS. All 7 Setting 'PullRequestSha' lines give this head, 900fead, and Configure printed "Left out 2 macOS entries". Each of the six Build and Test jobs, Ubuntu, Ubuntu ARM and Windows on Node.js 22 and 24, passed 1,497 unit tests with 2 skipped and 96 integration tests with 4 skipped.

macOS was left out of this draft's full build under the 9 November decision. The rc head is built with macOS before it merges into main.

What remains

Nothing for these two requirements.

@jwrosewell
jwrosewell marked this pull request as ready for review November 14, 2026 23:04
@jwrosewell
jwrosewell merged commit cdbbee8 into rc Nov 14, 2026
2 checks passed
@jwrosewell
jwrosewell deleted the fix/cloud-warnings branch November 14, 2026 23:04
@jwrosewell jwrosewell mentioned this pull request Nov 15, 2026
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