Repository navigation
Write each weighted JSON item with rawweighting and value, keeping weight and rawWeight for this release - #139
Merged
Merged
Conversation
…nce Element, and how the JSON writes lists of Element Data, plain nulls and keys
… data, and write each item of a list of Element Data by the JSON rules
…he specification's forms
…awweighting and value
…, and leave WKT values as they are
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 JSON Builder writes a Property's value as the Element Data gives it (
pipeline/fiftyone.pipeline.core/jsonbundler.jsline 271 at 7c99e780), with only a list of Element Data rewritten (lines 242 to 246). So each item of a weighted value in the JSON sent to the browser has the members the Element Data uses,value,weightandrawWeight(ip-intelligence/fiftyone.ipintelligence.onpremise/swigHelpers.jsline 59), where the specification writesrawweightingandvalue, in lower case. A cloud weighted value carriesrawweightingas well since #124, and an on-premise one does not.What the specification says
From
pipeline-specification/features/weighted-values.mdlines 45 to 58 at 706de1f0:What changed
The JSON Builder writes each item of a weighted value, being a list whose items each have a
valueand a numericrawWeightorrawweighting, withrawweightingandvalue, takingrawweightingfromrawWeightwhere an item has it. Each item keeps theweightandrawWeightit had beside them, deprecated, so client code that reads them still works in this release. Any other list is written item by item as #118 writes it, and any other value as it is. The Element Data is unchanged, so only the JSON sent to the browser changes, and nothing intypeschanges.Before you upgrade
Nothing breaks. Client-side code that reads
weightorrawWeightfrom a weighted value in the JSON, for example throughfod.complete, should readrawweightingandvalueinstead, becauseweightandrawWeightthere are deprecated and a later release removes them.Tests
The new
pipeline/fiftyone.pipeline.core/tests/jsonValueForms.test.jsbuilds a Pipeline with an element whose Property metadata and values are given, and reads the JSON the JSON Builder writes. It checks that a weighted string value given as the on-premise engines give one is written withrawweightingandvalueand keepsweightandrawWeight, that an item that already hasrawweighting, as the cloud engine's do, is written the same way, and that a string and a list of strings are written as they are.Red, run 43673171880 on 6c9edb3, these tests on
rc1ed293b, with only a throwaway workflow added that runs the core package's tests. On Ubuntu and on Windows 1 of 559 fails, being the test of a weighted value given as the on-premise engines give one, whichrcwrites with norawweighting. The test of an item that already hasrawweightingpasses onrc, which writes such an item as it is.Green, run 43673180942 on 79ad330, with only the same throwaway workflow added. All 537 pass on Ubuntu and on Windows, lint passes on the core package, and
npm run tscwrites the committed types again with no difference butpipeline/fiftyone.pipeline.did/types/owid-js/v1.d.ts, which #127 decides.Review then found two comments that still described the WKT change, which dddd7e9 corrects. Green on this head, run 43679273729 on dddd7e9, with only the same throwaway workflow added. All 537 pass on Ubuntu and on Windows, lint passes on the core package, and
npm run tscwrites the committed types again with no difference butpipeline/fiftyone.pipeline.did/types/owid-js/v1.d.ts.An earlier form of this change, which also wrote WKT values as objects and left out
weightandrawWeight, ran red in run 43656112507 and green in run 43659878531. Review asked for both names to stay for a release and for the WKT form to wait, as for #124.Full build
Run 43679502429 on
tmp/pr-matrix-139-d, started bytools/pr-matrix.pywithout macOS. All 7Setting 'PullRequestSha'lines give this head, dddd7e9, 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,606 unit tests with 2 skipped and 119 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
value-types.mdlines 123 to 130 at 706de1f0 ask for an object with one member,value.weightand the camel caserawWeightstay on each weighted item in the JSON.Both wait for a release that may change the form of a value, as #147 records, with the IP Intelligence cloud engine's own values in #138.