Page MenuHomePhabricator

Coerce datetimes to floats for PipelineRunStatsSnapshot
ClosedPublic

Authored by max on Dec 17 2019, 1:15 AM.

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

max created this revision.Dec 17 2019, 1:15 AM
schrockn requested changes to this revision.Dec 17 2019, 5:22 PM

What is the context here (other than error?) What changed? Just accepting both types seems to be putting lipstick on a pig here.

This revision now requires changes to proceed.Dec 17 2019, 5:22 PM
max added a comment.Dec 17 2019, 7:25 PM

The issue here is that the graphql layer and all of the javascript still wants float timestamps, but we have switched to proper datetimes lower down.

max added a comment.Dec 17 2019, 7:36 PM

I'm all for having timezone-awareness in the frontend code, but I don't think that's the right thing to bite off here.

What I'm asking is that we decide on what the right data to store in PipelineRunStatsSnapshot and then fix it in a more principled way. Shouldn't just be magically coercing things in the ctor

max added a comment.Dec 17 2019, 9:58 PM

we can do this in two places instead (the instantiations of PipelineRunStatsSnapshot), but that doesn't seem better. in the long run, it would be better for PipelineRunStatsSnapshot to contain datetimes and for them to go over the wire as something other than floats, so that a timezone-aware frontend library can deserialize them as something other than a raw timestamp.

It's definitely better to do type conversions at the appropriate callsites so that the contract for this particular class is more clear

schrockn resigned from this revision.Dec 18 2019, 1:06 AM

I'm going to let @nate do a final pass at this one because he understands the subtleties of different time representations much better than i do

nate accepted this revision.Dec 18 2019, 8:12 PM

LGTM. Note that per https://stackoverflow.com/questions/6999726/how-can-i-convert-a-datetime-object-to-milliseconds-since-epoch-unix-time-in-p#comment65890915_11111177 - we need to ensure the datetimes used here are "dumb" and not timezone-aware or datetime_as_float() will throw an exception; shouldn't be an issue since we're controlling the datetime generation.

This revision is now accepted and ready to land.Dec 18 2019, 8:12 PM