Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
troyraen opened this issue May 4, 2023 · 2 comments
Open

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

troyraen opened this issue May 4, 2023 · 2 comments
Assignees
Labels
Maintenance Maintain function or increase stability

Comments

@troyraen
Copy link
Collaborator

troyraen commented May 4, 2023

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 #187 (comment)

@troyraen troyraen added the Maintenance Maintain function or increase stability label May 4, 2023
@wmwv
Copy link
Collaborator

wmwv commented May 5, 2023

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
Copy link
Collaborator Author

troyraen commented May 7, 2023

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Maintain function or increase stability
Projects
None yet
Development

No branches or pull requests

3 participants