EngineResult

[mpharrigan]

plumb through some job info like job_id and job_finished_time.

cc @maffoo

[maffoo]

This looks good to me, just a few minor comments. Happy to approve once you convert from draft (and add tests?).

[dstrain115]

Fine with the general idea once we resolve comments and add tests, as maffoo mentioned.

[mpharrigan]

thanks for the review @dstrain115 and @maffoo . As a draft PR, just wanted to provide early insight into what sublassing Result would look like inside cirq_google. If the overall strategy looks good, I'll flesh out the PR with all the usual niceties

[maffoo]

Mostly looks good, though I have some concerns about serializing datetime.datetime. Maybe that could be split out into a separate PR?

[maffoo]

I would suggest serializing the timestamp instead:

Suggested change

	if isinstance(o, datetime.datetime):
	if o.tzinfo is not None:
	# coverage: ignore
	raise TypeError("datetime tzinfo is not serializable")
	return {
	'cirq_type': 'datetime.datetime',
	'year': o.year,
	'month': o.month,
	'day': o.day,
	'hour': o.hour,
	'minute': o.minute,
	'second': o.second,
	'microsecond': o.microsecond,
	'fold': o.fold,
	}
	if isinstance(o, datetime.datetime):
	if o.tzinfo is not None:
	# coverage: ignore
	raise TypeError("datetime tzinfo is not serializable")
	return {
	'cirq_type': 'datetime.datetime',
	'timestamp': o.timestamp(),
	}

For deserialization, you'll need to call fromtimestamp instead of the class constructor:

datetime.datetime.fromtimestamp(json['timestamp'])

Note that without tzinfo, serializing datestamps with calendar values like year, month, day means the result will be interpreted in the locale timezone at both serialization and deserialization time, which may be different. The timestamp, on the other hand, is based on the unix epoch, so this will ensure that the deserialized time is the same instant, even though it might have different calendar entries after deserialization if the locale timezone is different. Also, with timestamp you don't have to assert that o.tzinfo is None because both tz-aware and "naive" datetimes have a timestamp.

[mpharrigan]

I'll split this out. I think it's really important to be able to serialize timestamps and it's crazy we're missing this functionality.

Having looked a little closer: what do you think about only allowing the serialization of UTC datetimes? when tzinfo=None, there's a ton of ambiguity and it seems harder to accurately round-trip datetimes via unix timestamp, which converts to utc

[mpharrigan]

    naive_ts = datetime.datetime.now()
    utc_ts = naive_ts.astimezone(datetime.timezone.utc)
    assert naive_ts != utc_ts, 'tzinfo'
    assert naive_ts.timestamp() == utc_ts.timestamp()
    assert (utc_ts.day, utc_ts.hour) > (naive_ts.day, naive_ts.hour), 'at least in PDT'

    re_utc = datetime.datetime.fromtimestamp(utc_ts.timestamp())
    re_naive = datetime.datetime.fromtimestamp(naive_ts.timestamp())

    assert re_utc == re_naive, 'roundtripping turns to utc'
    assert re_utc != utc_ts, 'roundtripping loses tzinfo'
    assert naive_ts == re_naive, 'works, but only because my local time isnt changing(?)'

[mpharrigan]

#5274

[mpharrigan]

@maffoo @dstrain115 ptal; this now is based on top of #5274 and only includes the EngineResult changes.

[mpharrigan]

A lot of the test changes are putting in an update_time to the mocked quantum jobs.

@wcourtney can you verify that completed jobs will always have this field supplied?

[maffoo]

Mostly looks good, though I have a general question about whether we should use tz-aware datetimes consistently, since that's what we support for serialization.

[maffoo]

This naive datetime will not be serializable, right? In that case I'd suggest adding a timezone, so we model best practices. Or maybe we should support serializing naive datetimes :-)

[mpharrigan]

In the constructor of cg.EngineResult, I canonicalize everything to "aware" utc datetimes, so this will still function.

Do you think we should only allow "aware" datetimes in EngineResult? Or just take whatever is input and face an error on serialization?

In any event; I'll change the test fixtures to use aware datetimes.

[maffoo]

It's probably worth a broader discussion how we want to handle datetimes. My personal opinion would be to allow naive and aware datetimes in serialization as well, since we can convert either to a timestamp in a well-defined way, just like we can normalize them in the EngineResult constructor. I agree for ergonomics it's nice to handle either when constructing EngineResult, but just as it's potentially surprising to not be able to do round-trip equality if you serialize a naive datetime and get back an aware one, it also feels suprising to change things when contructing an object:

dt = datetime.datetime.now()
res = cg.EngineResult(job_finished_time=dt, ...)
dt == res.job_finished_time  # False!

That's not such a big deal here since people will generally not be constructing EngineResult instances manually.

[mpharrigan]

Yes, I agree with everything you say. Let me see if I can write down the options in a way everyone can agree on and then we can pick one.

[mpharrigan]

see #5301

[mpharrigan]

I've removed the constructor-conversion from this PR. I recommend if we relax the serialization constraints we do that in a follow-up PR