search

mwvgroup / Pitt-Google-Broker

A Google Cloud-based alert broker for LSST and ZTF
https://pitt-broker.readthedocs.io/en/latest/index.html
4 stars 0 forks source link

Add a test that exercises the reason for the fastavro pin #193

Open troyraen opened 1 year ago

troyraen commented 1 year ago

Fastavro is pinned in at least one of the cloud function requirements files. Here is a test that demonstrates the need for the pin.

import fastavro as fa

# test alert stored with the repo
falert = "tests/test_alerts/ztf_3.3_1154308030015010004.avro"

# standard fastavro call to deserialize into a list of dicts
with open(falert, "rb") as fin:
    list_of_dicts = list(fa.reader(fin))

Wtih fastavro==1.4.4, this works. With fastavro==1.7.3, this fails with TypeError: float() argument must be a string or a number, not 'NoneType'.

This is very likely a ZTF-specific problem, due to the non-standard ordering of their Avro schema types (the same problem that causes us to do a "fix schema" step before storing the Avros in the bucket).

Originally posted by @troyraen in https://github.com/mwvgroup/Pitt-Google-Broker/issues/187#issuecomment-1517098084

wmwv commented 1 year ago

Would something like tests/test_ztf_fastavro.py be a good place?

This might set a broader trend of having survey-specific tests "tests/test*.py". The number of surveys we're going to support will be finite (<10) so I think it's fine to have tests just right there. And then it is important that we don't break things when updating for just one survey; and to identify cases where surveys might have incompatible requirements (great sadness).

troyraen commented 1 year ago

I believe this is the same issue raised in fastavro/fastavro#676. The issue is marked as resolved. I only skimmed the info, but seems like we should be able to use newer fastavro versions if we pass in the schema to the reader, which we don't currently do for ZTF (but do for elasticc because we have to, since those avro packets are schemaless).