Repository navigation
Take the data update messages and levels .NET and 51degrees-node write - #98
Merged
Merged
Conversation
Send If-Modified-Since as an HTTP date, wait for the data update expected time before the first poll, add update_from_memory, and log the actions with the messages and levels in the Logging section of the data updates specification.
… update examples With If-Modified-Since sent as an HTTP date, the Distributor answers 304 when the file is current, which the examples reported as a failure.
…ice messages, which the next commits mask
…automatic updates, a file data file is written, and content checks carry on
… with automatic updates, write update_from_memory's bytes to a data file kept on disk, and carry on without an MD5 header or with content that is not compressed
…ection failure keeps polling
Each event in the data update service now carries the message and level of the .NET data update service, which the Errors table of data-updates.md follows. A failed connection, a 429, a 403, any other status, a failed integrity check and a failed refresh move from Warning to Error and take .NET wording. A failed write to the data file becomes a Warning and the engine is still refreshed from the data in memory. Every check is reported to the update complete callbacks once it ends. A watcher event with no newer file reports not needed, and one that arrived whilst another was being applied reports in progress.
…her error DataUpdateService.cs logs an error from an update complete callback with MessageAutoUpdateUnhandledError at Error level. The test now asserts that text, so it fails while Python writes its own sentence.
data-updates.md at e89679e9 says the callbacks after the one that raised are not called. On update polling the error is logged with the last row's message, which is .NET's MessageAutoUpdateUnhandledError, and on a programmatic check it passes to the caller. The tests cover polling, a programmatic check and an update from memory, and that polling still schedules its next check. This commit is the tests alone, so they fail.
…ter it data-updates.md at e89679e9 says the callbacks after the one that raised are not called, and that on update polling and the file watcher the error is logged with the last row's message, .NET's MessageAutoUpdateUnhandledError. Passing the error to the caller of a programmatic check, an update from memory or the update on start-up waits for next week, so on those routes it is logged in the same way for now. This commit is the tests alone, so they fail.
DataUpdateService.cs wraps the callback loop in one try, so the first
callback that raises stops the rest, and LogUpdateException logs it with
MessageAutoUpdateUnhandledError, "An unhandled error occurred while
checking for automatic updates for engine '{0}'", at Error.
data-updates.md says the same. Python now does both, on every route, and
the next check is still scheduled.
data-updates.md at e89679e9, lines 476 to 485, says an error from a row at Error level is logged at Error as "Error during check for data update" with the error attached, and the error carries the row's text. An error of any other kind, a callback's included, is logged with the last row's message, also with the error attached. Python's logger takes a string, so the error follows the entry text, its cause after it as "Error detail:". This commit is the tests alone, so they fail.
An error from a row at Error level is now logged as "Error during check for data update", with the error after it, being the row's text and then its cause, as .NET logs a DataUpdateException with the exception attached. Any other error, a callback's included, is logged with the last row's message and the error after it. Python's logger takes a string, so the error follows as "Error detail: <cause>", as the other Python messages put a cause. The cause of a failed connection is masked, as it can name the URL with its licence key.
51degrees-node writes an error from a row of the errors table as "Error during check for data update. Error detail: <row text>", with ". Error detail: <cause>" after it when there is a cause, and nothing more when there is none. The entry constant now holds that text, and a new test writes out two entries in full, one without a cause and one with. This commit is the tests alone, so they fail.
An error from a row of the errors table is now logged as "Error during check for data update. Error detail: <row text>", followed by ". Error detail: <cause>" when there is a cause, which is the text 51degrees-node writes at f471d65d (checkErrorText and withCause in dataFileUpdateService.js), so the two languages' logs match character for character.
…es them
51degrees-node at f471d65d writes "Updating on start-up", as the
data-updates.md table at e89679e9 does, names the one try in a refresh
after a check ("... after 1 tries.") and ends a refresh from memory with a
full stop, and writes the integrity row as the table does, with no space
before "data update". The tests now expect those texts, so they fail.
"Updating on start-up" takes the hyphen the data-updates.md table at
e89679e9 and 51degrees-node write. A failed refresh after a check or a
watched change names its one try ("... after 1 tries."), and one from
update_from_memory ends with a full stop, as the table's two rows and
51degrees-node's refreshDataFile do. The integrity row has no space before
"data update", as the table writes it.
Merged
…d file is not reported A data file kept on disk is read back by its engine, so the tests now use a data file whose refresh reads the file, as the on-premise engines do. They cover a download whose write fails, a temporary copy that cannot be written, an update from memory whose write fails, and a watched file nobody has written to.
…the changes the watcher finds The engine of a data file kept on disk reads that file, so letting a failed write through reported a success with the old data still in use. Both routes now end with the failure, under the errors table row for the copy that replaces the file, or the row for any other error when the temporary copy is the one that cannot be written, which is what 51degrees-node writes. The watcher looks every ten seconds where .NET and 51degrees-node are told by the operating system, so a look that finds the file untouched is not an event and is not reported. The statuses take the numbers .NET gives them.
The errors table in the data updates specification says an error of any other kind is logged with the error attached, and that a callback's error is logged in the same way. __emit wrote the message and bound no variable, so the error the callback raised was lost. It is now written after the message, which is what .NET, Java, Node.js and Go each do. The new test file takes the licence header every other file in the repository carries.
The comment on the expected message said .NET, Node.js and Go write the error the callback raised, and Java as well. Java's data update service calls each onUpdateComplete in a bare loop with no catch (DataUpdateServiceDefault.java on its rc, the loops after the update and after the file watcher's refresh), so a callback that raises there is not caught and nothing is logged. The comment now names the three that do.
This draft is stacked on the data update alignment, which has moved on since this branch took it. Its new work is the error a callback raised written after the entry, and the comment naming the three languages that attach it. The one conflict is in __emit, where both branches write the cause. The shared _log_unhandled stays, because it builds the same entry through _with_cause, which is the text 51degrees-node writes.
The merge of the data update alignment brought a second constant for the same entry. This branch already built the text with the cause attached, so appending the cause again made the expectation read it twice. The one constant carries the whole entry, and the comment saying why the cause is attached stays with it.
…ssages # Conflicts: # pipeline/fiftyone_pipeline_engines/src/fiftyone_pipeline_engines/datafile_update_service.py
The errors table pairs the any other error message with AUTO_UPDATE_UNKNOWN_ERROR, and a temporary copy that cannot be written has no row of its own, so it is that error. The test asserted the copy-over-the-existing-file status instead.
The write failure logged the any other error message and returned the copy-over-the-existing-file status, which the errors table pairs with a different message. Each branch now returns the status its own row gives.
Takes rc at 435274d, which now holds #69 (squashed as 8b0b624), #70, #59, #89 and #106. Three files conflict, being datafile_update_service.py, test_datafile_update_http.py and test_datafile_update_service.py, each resolved by keeping this branch's version. rc's version of each of the three is identical to #69's head e770a88, which this branch already took in at efbfb6d, so nothing rc brings to those files is lost. In __emit, rc logs the unhandled error inline and this branch calls _log_unhandled, which writes the same sentence with the same masking. Every other file rc changed takes rc's version, including the comment #70 changed in the device detection datafileupdate_console.py example.
jwrosewell
marked this pull request as ready for review
November 12, 2026 19:19
This was referenced Nov 12, 2026
Merged
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.
This draft follows .NET's data update messages and levels in every place but two, the update on start-up message and the refresh attempt message, where it keeps the specification table's text, as 51degrees-node does.
The problem
Python's data update service wrote its own wording for each event, and logged most failures at Warning where .NET logs them at Error. A customer reading a Python log and a .NET log of the same failure saw two different messages, so 51Degrees support could not match one to the other. Some events .NET reports were not reported at all, and a check that needed no update was often reported as nothing rather than as not needed.
The decision
James Rosewell, 10 November 2026, decision 7: "The data-updates.md Errors table follows .NET's messages and levels", and the other languages align to them now, because "This is all about alignment." Java, Node, Python, Go and Rust use .NET's text and level for each event, in this release. His decision 2 covers the reporting, which is that every check is reported, and a skipped or not-needed check is reported as NotNeeded, as .NET's
DataUpdateService.csdoes.What the specification says
data-updates.md at e89679e9 says this at lines 475 to 486.
and "Either way the callbacks after the one that raised the error are not called". The Errors table that follows, lines 488 to 505, gives each row's text, level and status.
What the other languages did
pipeline/Engines/Services/DataUpdateService.csat b3e90b4, which is the file the specification's table was written from.Python's logger takes a string, so the error is written after the entry text. The text is the one 51degrees-node writes at 74cdc781 (
withCause,checkErrorTextandunhandledErrorText, lines 58 to 110), character for character, being:Error during check for data update. Error detail: <row text>, then. Error detail: <cause>when there is a cause (a single space beforeError detail:when the row text already ends with a full stop), and nothing more when there is noneAn unhandled error occurred while checking for automatic updates for engine '<Engine type>'. Error detail: <cause>Event by event
Updating on start-up, Info, as the table at e89679e9 line 448 and 51degrees-node write it. .NET writesUpdating on startup.Attempting to refresh Engine '<name>' with new data, Info, as the table at e89679e9 line 456 and 51degrees-node write it. .NET writesAttempting to refresh engine with new data, naming the engine in a structured field.Error accessing data update service at '<url>' for engine '<name>', Error, with the cause maskedNo response from data update service at '<url>' for engine '<name>', ErrorToo many requests to '<url>' for engine '<name>', ErrorAccess denied to data update service at '<url>' for engine '<name>', ErrorHTTP status code '<code>' from data update service at '<url>' for engine '<name>', ErrorContent-MD5headerNo MD5 hash included in data update response for '<url>'. Unable to verify data integrity, WarningIntegrity check failed. MD5 hash in HTTP response '<response hash>' for '<name>'data update does not match calculated hash for the downloaded file '<calculated hash>'., Error, with no space beforedata update, as .NET (DataUpdateService.cslines 1461 to 1465 at b3e90b4) and the table write itThe data file '<identifier>' is checking for updates but does not have a temporary file path configured., Error, statusAUTO_UPDATE_TEMP_PATH_NOT_SETAn error occurred when copying a data file to replace the existing one at '<path>'., Error, with the cause, statusAUTO_UPDATE_NEW_FILE_CANT_RENAME, and the update ends, as 51degrees-node writes it at 74cdc781, lines 664 to 676AUTO_UPDATE_UNKNOWN_ERROR, and the update ends. The table pairs that message with that status at e89679e9 line 505, and .NET pairs them too, its genericcatch (Exception)settingAUTO_UPDATE_UNKNOWN_ERROR(DataUpdateService.cslines 886 to 890 at b3e90b4) whereAUTO_UPDATE_NEW_FILE_CANT_RENAMEis set only for the copy over the data file at line 1069. 51degrees-node writes the same message with the rename status instead (74cdc781, lines 735 to 742), so this is the one place where following .NET and the table means not following 51degrees-node, and the gap in 51degrees-node is under What remains.An error occurred when copying a data file to replace the existing one at '<path>'., Error, with the causeAn error occurred when applying a data update to engine '<name>' after 1 tries., Error, with the cause. Python makes one try and names it, as 51degrees-node does, where .NET makes up to 10 (lines 1190 to 1216 at b3e90b4).An error occurred when applying a data update to engine '<name>'., Error, with the causeAUTO_UPDATE_UNKNOWN_ERROREach row at Error level is written under
Error during check for data update. Error detail:, as above.Reporting, under decision 2.
Effect on callers
AUTO_UPDATE_NEW_FILE_CANT_RENAME, where before this draft it ended with an error nobody caught. The engine keeps the data it has.AUTO_UPDATE_UNKNOWN_ERROR, which is the status the errors table gives the message that failure logs.What changed
All in
pipeline/fiftyone_pipeline_engines/src/fiftyone_pipeline_engines/datafile_update_service.py. This draft was stacked on #69, which merged intorcat 15:06 UTC on 12 November as 8b0b6248.rcis now merged into this draft, so its diff againstrcholds this draft's own changes alone, in that file and its two test files. How the merge was resolved is under Mergingrcbelow.None, and a data file that asks for a temporary copy but has no temporary directory, are reported._log_check_errorwrites a row at Error underError during check for data update. Error detail:with the cause after it, and_log_unhandledwrites the last row's message with the cause, as above. The cause of a failed connection is masked, as it can name the URL with its licence key._refreshnames the one try (REFRESH_TRIES) for a refresh after a check or a watched change, and ends with a full stop for one fromupdate_from_memory._write_data_filereturns a status for every failure and the caller ends the update, because the engine of a data file kept on disk reads that file. Writing straight over the data file carries the errors table's row for the copy that replaces it, withAUTO_UPDATE_NEW_FILE_CANT_RENAME, and a temporary copy that cannot be written carries the last row, withAUTO_UPDATE_UNKNOWN_ERROR. Both callers pass the status back unchanged, the download route through_update_failedandupdate_from_memoryto its caller._with_causemasks the cause, so a licence key cannot reach a log entry through an error that names the update URL, whichever of the two entry texts carries it. That came in with #69, which masked the one place it wrote the cause, and this draft writes both places through one method.track_local_data_file_changesreports the changes it finds and says nothing about a file nobody has written to.UpdateStatusgainsAUTO_UPDATE_TEMP_PATH_NOT_SETandAUTO_UPDATE_UNKNOWN_ERROR, at 12 and 13, which are the numbers .NET gives them inIDataUpdateService.csat b3e90b4. 11 is .NET'sAUTO_UPDATE_NO_CONFIGURATION, which this service has no route to, andAUTO_UPDATE_TIMEOUTis left out for the same reason._retry_textis removed, with the only message that used it gone.Two places where this service cannot do what the table says
The Warning row is not written, because Python cannot refresh a disk engine from memory, so that row of the table has no implementation here, and it is written out in full below rather than left silent. The row "Writing data passed in from memory to the data file failed" (e89679e9 line 501) is Warning, with the Engine still refreshed. .NET earns that by calling
RefreshData(identifier, data)with the data in hand (DataUpdateService.cslines 394 to 420 at b3e90b4). In Python a data file withdownloadset refreshes throughengine.refreshData(), which reads the file on disk (devicedetection_datafile.pylines 107 to 119 andipintelligence_datafile.pylines 102 to 114), so after a failed write the engine would be left with the old data whilst the callbacks were told the update had succeeded. Both routes therefore end with the write failure. Meeting the row would mean giving the data file's refresh the bytes to use, in the device detection and IP Intelligence packages, which is listed under What remains. 51degrees-node does not meet the row either, itsupdateFromMemoryrefusing a data file that has a path at all (74cdc781, lines 768 to 776), where this draft writes the file and then refreshes from it. So the row asks for behaviour that only an Engine able to take the new data directly can give, which is what .NET'sRefreshData(identifier, data)does, and no other language it was written from meets it. The table came from specifications#60, merged on 11 November, and whether this row should stand as written needs a change there rather than one here.A look at an untouched watched file is not reported. e89679e9 lines 345 to 355 say a check by the file watcher MUST be reported once it ends, and count "a downloaded or watched file that is not newer than the current file" as not needed. In .NET and 51degrees-node a watcher check happens only when the operating system says the file was written, so that row covers a second event for one write. Python has no such watcher and looks every 10 seconds (
FILE_SYSTEM_WATCHER_INTERVAL_SECONDS), so reporting each look would tell every callback six times a minute, for ever, that no update was needed, which neither of the two languages this draft follows does. A look that finds the file written to is the event here, and it is reported. Overturning this reading costs one line intrack_local_data_file_changesand the test beside it.Merging
rc#69 and #70 merged into
rcat 15:06 and 15:08 UTC on 12 November. #69 was squashed into 8b0b6248, sorcholds none of the commits this draft took from #69's head, and the draft no longer merged. a934ee1c mergesrcat 435274d8 into the draft with a merge commit, whose first parent is the earlier headcc916dba.Three files conflicted, being
datafile_update_service.py,test_datafile_update_http.pyandtest_datafile_update_service.py, and each keeps this draft's version. Nothing ofrcis lost by that, becauserc's version of each of the three is byte for byte the version at #69's head e770a881, which this draft took in atefbfb6d8and built on. #69's squash is the only commit onrcsince the merge base8ac72361that touches any of the three, so every linercchanged in them is already here, and the conflicts come only from this draft changing the same lines further. The three files on this head are the same blobs as oncc916dba.The conflict in the service is in
__emit, whererclogs an unhandled error inline and this draft calls_log_unhandled._log_unhandledand_with_causebuild the same sentence with the same masking, "An unhandled error occurred while checking for automatic updates for engine '<type>'. Error detail: " followed by the masked cause.The guard under "exit if already updating",
if data_file.updating: pass, is the same on both sides (lines 122 to 125 here, lines 116 to 119 onrc), so the merge leaves it as it was. #87 is the draft that makes that guard do something, and the two drafts meet there.Every other file
rcchanged takesrc's version. That includes the comment #70 changed in the device detectiondatafileupdate_console.pyexample, which this branch still had in #69's wording.test_licence_key_masking.pyand the IP Intelligencedatafileupdate_console.pyexample were already the same on both sides.Runs
The runs below are on head
a934ee1c338e5d0395a3cdfacb2bf0699bf696ec, which is the earlier headcc916dbawithrcat 435274d merged in, as described above. I read every run's log rather than its result, and each temporary branch is one commit on the commit named.rc, run 43283805955, failure. The branchtmp/py98rc-redate4910be5isrc435274d plustest_datafile_update_http.pyandtest_datafile_update_service.pyfrom this head and the temporary workflow, with no source change. 18 failed and 31 passed in the two files. The failures includetestAFailedTemporaryCopyIsAnyOtherError("type object 'UpdateStatus' has no attribute 'AUTO_UPDATE_UNKNOWN_ERROR'"),testTheErrorEntryIsWrittenExactly,testNoTemporaryPathIsReportedand each test of a failure's message and level, whichrcstill writes at Warning in its own words. The 10 licence key masking tests and the 6 other engines tests passed, because Align the data update service to the specification: HTTP date, expected time, update from memory and logging #69 brought the masking intorc, and that is also why fewer fail here than the 38 that failed on the olderrc.tmp/py98rc-greenat94069893is the head plus the temporary workflow and nothing else. 49 passed in the two data update test files, 10 in the licence key masking tests and 6 in the rest of the engines tests.tmp/pr-matrix-98e, success, 12 jobs passed and 2 skipped. I read its log rather than its result, and its 11Setting 'PullRequestSha'lines all reada934ee1c338e5d0395a3cdfacb2bf0699bf696ec, with Configure printing "Left out 5 macOS entries".Earlier runs, each on the commit named and all on heads this draft has moved past, are kept so the order the work was done in can be read. The four on
cc916dbacover the same three files as this head.cc916dbawithout its last commit, run 43198709124, failure, 1 failed and 48 passed. The branchtmp/py98-red4atfaaa90eaheld every source change in this draft but the one that pairs a failed temporary copy with its status, and the one failure istestAFailedTemporaryCopyIsAnyOtherError.rc8ac7236 with the tests alone, run 43198642071, failure, 38 failed and 11 passed, then 2 failed and 8 passed in the masking tests. Green oncc916dba, run 43198739721, success. Full build ofcc916dba, run 43198549171, success, 11Setting 'PullRequestSha'lines all that head.testTheErrorEntryIsWrittenExactly, which writes two entries out in full, one without a cause and one with. Green on 082c572, run 43000461334, 46 passed.Setting 'PullRequestSha'lines all that head. Its red and green pair on that head are 43177129799 and 43177154341.3395e52leaves one constant holding the whole entry, which is what the service writes.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
AUTO_UPDATE_NEW_FILE_CANT_RENAME(74cdc781, lines 735 to 742), where the table and .NET pair that message withAUTO_UPDATE_UNKNOWN_ERROR. That needs its own draft in 51degrees-node, which this draft does not open.'<Engine type name>'in the Errors table and'<Engine type>'in the other tables name the same thing twice, which is another wording for the specification to settle and which no language reads differently.The last release into
mainmerged on 2026-11-04.