avro reader integration tests - #7156
Conversation
|
rerun tests |
1 similar comment
|
rerun tests |
cwharris
added
4 - Needs cuIO Reviewer
cuIO
Codecov Report
@@ Coverage Diff @@
## branch-0.19 #7156 +/- ##
==============================================
Coverage ? 82.22%
==============================================
Files ? 100
Lines ? 16969
Branches ? 0
==============================================
Hits ? 13953
Misses ? 3016
Partials ? 0 Continue to review full report at Codecov.
|
vuule
left a comment
vuule
left a comment
There was a problem hiding this comment.
Good stuff. Got some questions/suggestions.
| records = [ | ||
| {"prop": avro_val}, | ||
| {"prop": None}, | ||
| ] |
There was a problem hiding this comment.
is the dataframe shape (1,2)?
There was a problem hiding this comment.
expected and actual are the same shape. I don't know what shape that should be.
There was a problem hiding this comment.
Should we also have some tests with a large number of rows?
There was a problem hiding this comment.
We can test a large number of values,. It would be nice to have a test data generator. I see we're generating random values for fuzz testing. Are we able to do that in a deterministic manner so it can be also be used for unit tests?
There was a problem hiding this comment.
IIRC the data generator optionally takes a seed value; that the output is deterministic for each seed. CC @galipremsagar for pointer to the generator + sample use.
There was a problem hiding this comment.
Since we are discussing having large rows, I'd recommend staying in <30 rows range to not slow down things in pytests by a lot as that would slow down in gpu CI too. If there is a bug that only reproduces for a large column scenarion then we can widen the test coverage for large columns, else I think fuzz tests should take care of large rows testing. For using the dataset generator, here is how we can use it:
>>> import cudf
>>> from cudf.tests.dataset_generator import rand_dataframe
>>> rand_dataframe(dtypes_meta=[{"dtype": "int64", "null_frequency": 0.4, "cardinality": 10}], 100, seed=2)
File "<stdin>", line 1
SyntaxError: positional argument follows keyword argument
>>> rand_dataframe(dtypes_meta=[{"dtype": "int64", "null_frequency": 0.4, "cardinality": 10}], rows=100, seed=2)
pyarrow.Table
0: int64
>>> cudf.DataFrame.from_arrow(rand_dataframe(dtypes_meta=[{"dtype": "int64", "null_frequency": 0.4, "cardinality": 10}], rows=100, seed=2))
0
0 -1468954783236838137
1 <NA>
2 2200161065918338095
3 -1193091257902529461
4 -5448271019629827509
.. ...
95 <NA>
96 2200161065918338095
97 -8745117541724490168
98 <NA>
99 -4301277553722975852
[100 rows x 1 columns]Alternatively, There is also an existing API that also returns deterministic data with the same seed values that is widely used across our pytests:
https://github.com/rapidsai/cudf/blob/branch-0.18/python/cudf/cudf/datasets.py#L60
This is much simpler to use and fits the use-case here.
There was a problem hiding this comment.
Should we rather just change this test to be a list of values(cudf_val be length 5/10) instead of 1 value?
|
Looks like the PR is failing due to mypy style checks unrelated to these changes. Can we ignore that? |
Fix incoming: #7279 |
vuule
left a comment
vuule
left a comment
There was a problem hiding this comment.
Requesting changes based on the two unresolved comments.
|
rerun tests |
cwharris
removed
the
0 - Waiting on Author
|
Retargeted to branch-0.19. |
galipremsagar
left a comment
galipremsagar
left a comment
There was a problem hiding this comment.
Maybe better to add some PR description?
galipremsagar
added
5 - Ready to Merge
|
@gpucibot merge |
4 participants
Added some avro reader integration tests for fastavro. These cover type detection, single-value parsing, and null value parsing, but do not cover parsing multiple values.