fix: deserializing instantiates seed columns twice #188
Conversation
andreatgretel
requested review from
eric-tramel,
johnnygreco,
mikeknep and
nabinchha
eric-tramel
left a comment
eric-tramel
left a comment
There was a problem hiding this comment.
Quick fix on blocking bug, thanks @andreatgretel !
| if not isinstance(col, SeedDatasetColumnConfig): | ||
| builder.add_column(col) |
There was a problem hiding this comment.
Not sure of the urgency here, if we'd want to land this and ship another patch release or what, but: I believe this entire problem should disappear (and this specific highlighted code become a no-op) once #167 lands. As part of those changes, we no longer save SeedDatasetColumnConfig columns on the config builder—it only has the SeedConfig, including through the config serde process.
There was a problem hiding this comment.
Oh that's a great point
Maybe we can proceed as with the fix yesterday - land this quickly on a 0.2.x release, and leave other changes for 0.3?
Agree it's up to how urgent this is and whether we want to do a new 0.2.x release, will let @eric-tramel and @johnnygreco coordinate on that.
There was a problem hiding this comment.
Excited for the much needed rework @mikeknep :)
For this case, simple process in my opinion:
- Main is bugged, should fix main quickly with this edit, ship a minor version today/tomorrow to unblock downstream users immediately.
- Update working-state main with new refactor and ship on its own schedule.
There was a problem hiding this comment.
^ sounds like a good plan 👍
I can publish the minor patch today.
andreatgretel
deleted the
andreatgretel/fix/deserialization-and-seed-columns
branch
See #187 for context.
This fixes the issue by adding a check before running
builder.add_column- if column is aSeedDatasetColumConfig, that line is skipped.Added regression test.
Closes #187.