Query Builder tests for OOM and memory limit (from PR #2053)

[grusev]

Reference Issues/PRs

What does this implement or fix?

Any other comments?

Checklist

Query Builder tests for OOM and memory limit

The PR contains code that introduces memray and pytest-memray libraries. With their help it is possible to create 2 types of tests:

• memory leaks test - the memory leaks tests will accept one or two parameters:

  • mem leak threshold - a threshold of mem in KB/MB that will trigger failure if above
  • (optional) filter function - the function can be passed to memray to filterout code that we do not want to be accounted as mem leak. In future we might get some code that we KNOW has memory leaks but is outside of our scope. Thus we can use this function to filter it out.. Currently the code accepts that mem leaks under certain treshold will not lead to failure. If that threshold is vialoted in future this should mean immediate development investigative task. In the log there will be information what is the code that triggered that. IMPORTANT: code source will be if the test is executed against release build - only the size of leak will be correct. To know the source of the code the test must be executed against build with debug information

• memory limit test - the memory limit test is about enforcing memory efficiency of code. Currently I have set CURRENT memory limit as limit for the this QueryBuilder code. If test fails in future then that would mean that our code is LESS memory efficient in that code. Why that has happened needs to be investigated Also, the number currently set accepts situation AS IS. It does not makes claim that we are memory efficient. No such study has been done it is outside of the test purpose. If such thing is needed then an investigative task need to be created

• Have you updated the relevant docstrings, documentation and copyright notice?

• Is this contribution tested against all ArcticDB's features?

• Do all exceptions introduced raise appropriate error messages?

• Are API changes highlighted in the PR description?

• Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?

[vasil-pashov]

Why is this brackets? It's inconsistent with query_resample from this file and most of the codebase.

[grusev]

a way to have decent multilines ...

[vasil-pashov]

I don't think you need then in this case. The function quoted above query_resample doesn't use brackets. We should keep the style homogeneous.

[vasil-pashov]

I don't think this is needed. It should be deleted when it goes out of scope.

[grusev]

In theory yes. In practice not sure. Thus one delete more is not hurting

[vasil-pashov]

I think we should have good understanding of the code we're putting both in theory and in practice. It's unusual to delete stuff in python in such way. We should prove there is a reason to do this and document why it is needed. If not we should omit it.

Let's see @alexowens90's take on this as well.

[vasil-pashov]

Why are some things type annotated e.g. lib: Library while others are not e.g. df is not df: pd.DataFrame

[grusev]

ommissions, or somewhere not needed. fixed here and there

[alexowens90]

The chances of this filtering anything out are very low, is that deliberate?

[grusev]

yes, this is deliberate to impose attempt to filter by impossible condition which should still be evaluated. And as we know with strings things are always buggier than with numbers

[alexowens90]

My issue is that the condition is not impossible, just unlikely. Making it actually impossible is fine (e.g. use a different character set than the generated dataframe), but we have optimisations in place for when nothing is filtered out, which could then skew the results if you get unlucky and the dataframe does contain QASDFGH

[grusev]

Yes. It is very unlikely. Thus making the results 99.99% of the dataframe (note that by default I think we generate str of 10 chars array and this is 7 chars. So perhaps really impossible to achieve unless bug

Anyway for our purposes we do not need exactness at all. Thus actual returned result if it is 99 or 100% of rows really does not matter

[alexowens90]

It does matter. Returning 100% and 99.999% of a dataframe can take totally different code paths

[vasil-pashov]

Let's see what @alexowens90 thinks

[grusev]

added aditional query also without any filter. Thus we have now 3 queries that have groupby agg., with filter(50%) filter(100%) effectivly and no filter ...

there is additional functional test also that covers the case if there is something fishy with queries

[alexowens90]

@grusev the query needs to be changed to one of:

• Return 100% of the dataframe
• Return <100% of the dataframe

And it has to be consistent. We have optimisations in place so that a filter that doesn't remove anything is much more efficient than a filter that removes even 1 row.

[grusev]

We have it already .. look at:

def query_no_filter_only_groupby_with_aggregations() -> QueryBuilder:
"""
groupby composite aggregation query for QueryBuilder memory tests.
The query basically will do aggregation of half of dataframe
"""
q = QueryBuilder()
return (
q.groupby("uint8")
.agg({"uint32": "mean",
"int32": "sum",
"strings": "count",
"float64": "sum",
"float32": "min",
"int16": "max"})
)

[grusev]

only the comment is misleading but all is ok

[vasil-pashov]

This will probably disable starting from 0-th row. Is this the desired behavior?

The same question applies to query_date_range_57percent

[vasil-pashov]

1. As the row range is closed on the left side and opened on the right side it is safe to pass size here. It's also the correct thing to do because the current thing represents a the dataframe without it's last row.
2. Why is the conversion to int needed. Looking at the type hint size is already an integer.