Aggregate network objects

[OwlHute]

Closes #601

I have:

• Formatted any Python files with black
• Brought the branch up to date with master
• Added any relevant Github labels
• Added tests for any new additions
• Added or updated any relevant documentation
• Added an Architectural Decision Record (ADR), if appropriate
• Added an MPLv2 License Header if appropriate
• Updated the Changelog

Description

Add API route aggregate_network_objects()

[greenape]

👍

[greenape]

I've been giving the params for this some thought - I actually think what would be most useful is to have it take: network_objects, statistic, by. That can be achieved without touching the underlying Flowmachine class, by constructing a tno object, and then calling aggregate on it with the by and statistic arguments.

[OwlHute]

You'll have to expand on that a bit, in terms of roughly how the call would look. I gather tno is TotalNetworkObject, and "by" is possibly the date range?! But also, I thought you were trying to do away with calls to aggregate() (#599)

[OwlHute]

As you were, I understand now what you are driving at! (in view of the review comment further down)

[greenape]

Arguments need to be total_network_objects_query: dict, statistic and by: str (or a more explanatory name than by would also be fine!)

[greenape]

What's going on here?

[OwlHute]

This relates to your comment in #601 "Refactor AggregateNetworkObjects to take TotalNetworkObjects as main argument"

[greenape]

Ah. I think I've failed to communicate what I meant there - the suggestion was that rather than taking all the arguments necessary to create a TotalNetworkObjects object in the __init__ method, that this class should instead just take a TotalNetworkObjects object instead of those arguments.

This change actually alters the class hierarchy of the class (https://www.digitalocean.com/community/tutorials/understanding-class-inheritance-in-python-3 is a decent overview of class inheritance in python), and I imagine will cause a few issues because TotalNetworkObjects also inherits from Query, which would give you a somewhat odd inheritance hierarchy.

[OwlHute]

OK I'll amend the code accordingly. (I do understand python class hierarchies, but I'll have a quick skim of that page)

[codecov]

Codecov Report

Merging #607 into master will increase coverage by 0.05%.
The diff coverage is 94.62%.

@@            Coverage Diff             @@
##           master     #607      +/-   ##
==========================================
+ Coverage   93.38%   93.44%   +0.05%     
==========================================
  Files         122      123       +1     
  Lines        6199     6253      +54     
  Branches      666      668       +2     
==========================================
+ Hits         5789     5843      +54     
+ Misses        286      285       -1     
- Partials      124      125       +1

Impacted Files	Coverage Δ
flowauth/backend/flowauth/models.py	93.29% <ø> (ø)	⬆️
...ine/core/server/query_schemas/flowmachine_query.py	100% <100%> (ø)	⬆️
...core/server/query_schemas/total_network_objects.py	100% <100%> (ø)	⬆️
flowclient/flowclient/client.py	93.9% <100%> (+1.23%)	⬆️
.../server/query_schemas/aggregate_network_objects.py	100% <100%> (ø)
flowapi/flowapi/user_model.py	97.43% <100%> (+0.06%)	⬆️
...machine/flowmachine/core/server/action_handlers.py	94.07% <100%> (+0.2%)	⬆️
...machine/core/server/query_schemas/custom_fields.py	77.77% <76.47%> (+3.58%)	⬆️
...wmachine/features/network/total_network_objects.py	97.1% <94.44%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa9c6c0...79d179e. Read the comment docs.

[greenape]

Don't need a deepcopy here, just an assign is fine.

[greenape]

Below this, period will now need to refer to the period attribute of the passed in object.

[greenape]

This needs to match the AggregateNetworkObjects form now

[greenape]

Needs to have the right fields, one of which is a bit tricky because it should refer to another schema. See FlowsSchema, or ModalLocationSchema for an example of how to do that.

[maxalbert]

@OwlHute FYI, PRs #609 (spatial aggregate) and #611 (joined spatial aggregate) have now been merged, so hopefully that helps with this PR as well (see Jono's inline comment).

Github is indicating a couple of merge conflicts with this branch due to the changes in #609 and #611 so these will need to be resolved (but should be very simple).

[OwlHute]

Hi Max. I've merged the lastest "master" branch with my aggregate_network_objects private branch & pushed the latter to github for #607

Running the tests on my workspace, there is one failure. But I am not clear on the reason for this & could use a bit of help to diagnose & fix it. (I think the code is 99% OK and expect only a very small fix is needed, but the trick is knowing where and what! )

[maxalbert]

Thanks @OwlHute. Which tests are failing for you locally? When I click on the "Details" link next to the integration checks failure here on Github (see here) it shows two failing tests:

• tests/flowmachine_server_tests/test_server.py
• tests/query_tests/test_queries.py

The first one seems to be from a missing update to the API spec. Have you been able to get the approvaltests running locally? If so, it should be just a matter of running this particular test (e.g. via pytest -svx tests/flowmachine_server_tests/test_server.py from within the integration_tests pipenv environment) and it will update the spec for you (via a GUI popup in whichever diff viewer you have configured as explained in the developer docs), so you just need to apply the changes in the diff viewer and commit the result.

For the second failing tests, if you run it via pytest -svx tests/query_tests/test_queries.py (again from within the integration_tests pipenv environment, it will succeed for the first few parameters and then fail on the aggregate_network_objects parameter (also see the CircleCI failure linked above). This is due to some of Jono's recent changes in #609 or #611. If you take a look at how the other JSON parameters have changed now (with a toplevel "spatial_aggregate" key rather than having the query_kind directly as the toplevel key) you should be able to deduce how your addition must be tweaked to make the tests pass.

[OwlHute]

Thanks for the review comment Max. I have fixed the first error. However, I am still slightly puzzled by the second issue.

From the test output, the following is the query being sent to flowapi :

query_kind = 'aggregate_network_objects'
params = {'by': 'day', 'statistic': 'median', 'total_network_objects': {'aggregation_unit': 'admin3', 'end_date': '2016-01-02', 'query_kind': 'total_network_objects', 'start_date': '2016-01-01'}}

and referring to total_network_objects() in flowclient/flowclient/client.py that 'total_network_objects' dictionary is correct:

return {
    "query_kind": "total_network_objects",
    "start_date": start_date,
    "end_date": end_date,
    "aggregation_unit": aggregation_unit,
    "period": period,
}

It also matches the parameters in the AggregateNetworkObjects class in flowmachine/flowmachine/core/server/query_schemas/total_network_objects.py :

def init(self, *, total_network_objects, statistic="avg", by=None):
:::

Finally, in flowmachine/flowmachine/core/server/query_schemas/aggregate_network_objects.py the actual query is run as:

def _flowmachine_query_obj(self):
"""
Return the underlying flowmachine aggregate_network_objects object.

   Returns
   -------
   Query
   """
   tot_network_objs = self.total_network_objects._flowmachine_query_obj

  return AggregateNetworkObjects(
       tot_network_objs, statistic=self.statistic, by=self.by
   )

The point is "spatial_aggregate" is not present in the input query, only in the output results.

I assume the latter call must introduce SpatialAggregate into the mix, but I'm not clear where and what need amending for this. (Once I've fixed this issue, I should be able to handle any similar changes in future.)

It's easy to see what the code is trying to achieve in principle, but all the same the details are damned intricate!

[OwlHute]

I've got a bit further, and identified the reason for the test failure. (A bug in the test code prevents this being displayed properly!):

       try:
           error = response.json()["msg"]
           # payload_info = f" Payload: {response.json()['payload']}"
           payload_info = str(response.json()) + ' (response.status_code = ' + str(response.status_code) + ')'
       except ValueError:
           # Happens if the response body does not contain valid JSON
           # (see http://docs.python-requests.org/en/master/api/#requests.Response.json)
           error = f"the response did not contain valid JSON"
           payload_info = ""
       raise FlowclientConnectionError(
          f"Something went wrong: {error}. API returned with status code: {response.status_code}.{payload_info}"
       )

E flowclient.client.FlowclientConnectionError: Something went wrong: Aggregation unit must be > specified when running a query.. API returned with status code: 400.{'msg': 'Aggregation unit must be > specified when running a query.'} (response.status_code = 400)

The bug is that the dict returned by response.json() contains no 'payload' key, so the error was being displayed as just "KeyError: 'payload'".

So the next step is to find out where that message "Aggregation unit must be specified when running a query" is generated. (For the new aggregate_network_objects() API route, it is no longer appropriate!), This turns out to be in /flowapi/flowapi/user_model.py function _get_query_kinds_and_aggregation_units() and the JSON it objects to is:

{
'query_kind': 'aggregate_network_objects',
'total_network_objects':
{
'query_kind': 'total_network_objects',
'start_date': '2016-01-01',
'end_date': '2016-01-02',
'aggregation_unit': 'admin3'
},
'statistic': 'median',
'by': 'day'
}

and I can see nothing wrong with that query!

[maxalbert]

Thanks @OwlHute! Yes, you are right that the missing payload key was shadowing the actual error message.

I had a look and the error happens in flowapi/flowapi/user_model.py. The issue is that the code in UserObject._get_query_kinds_and_aggregation_units() assumes that unless the query_kind is spatial_aggregate or joined_spatial_aggregate, it must have an aggregation unit itself.

However, aggregate_network_objects isn’t a spatial aggregate (but rather a statistical aggregate) so it doesn't contain an aggregation_unit itself. The easiest fix that I can think of for the time being is to add another else branch which checks for aggregate_network_objects and extracts the aggregation_unit from the underlying total_network_objects (@greenape: any better ideas?).

This is a bit of a hack for the time being, but unless I'm missing something it would require larger refactorings to have a more elegant solution and I guess this is outside the scope of this particular PR. Let me know if this explanation makes sense and is sufficient to make the required tweaks.

[maxalbert]

Jono's latest changes require this to be a keyword argument now:

Suggested change

	tot_network_objs, statistic=self.statistic, by=self.by
	total_network_objects=tot_network_objs, statistic=self.statistic, by=self.by

[greenape]

Suggested change

	aggregate_by : str
	aggregate_by : {"second", "minute", "hour", "day", "month", "year", "century"

[greenape]

Suggested change

	- Added new flowclient API entrypoint, aggregate_network_objects(), to access (with simplified parameters) equivalent flowmachine query [#601](https://github.com/Flowminder/FlowKit/issues/601)
	- Added new flowclient API entrypoint, `aggregate_network_objects`, to access equivalent flowmachine query [#601](https://github.com/Flowminder/FlowKit/issues/601)

[greenape]

Can we have a changed entry here as well to reflect that the names of the arguments to TotalNetworkObjects and AggregateNetworkObjects have been altered?

[greenape]

Needs to go in the [Unreleased] section, and should mention the underlying TotalNetworkObjects and AggregateNetworkObjects classes

[greenape]

Sorry to be pedantic, but can this go in the changed section of [Unreleased], and can we explicitly refer to TotalNetworkObjects and AggregateNetworkObjects?

[greenape]

👍

[OwlHute]

The CHANGELOG Unreleased "Changed" entry now reads:

Changed

• The period argument to TotalNetworkObjects in FlowMachine has been renamed total_by
• The period argument to total_network_objects in FlowClient has been renamed total_by
• The by argument to AggregateNetworkObjects in FlowMachine has been renamed to aggregate_by
• The by argument to AggregateNetworkObjects in FlowMachine has been renamed to `aggregate_by

So I think in that 4th line AggregateNetworkObjects & "FlowMachine" should read `aggregate_network_objects" & "FlowClient" respectively