Federated Analytics and Histogram Taskrunner Workspace

[tanwarsh]

Summary

The PR contains changes for Federated Analytics and Histogram Taskrunner Workspace
as discussed in #1385 (comment)

without/minimum core components changes and dummy model.

Type of Change (Mandatory)

Specify the type of change being made.

• New Feature.

Description (Mandatory)

The PR includes following changes:

• Used Dummy model to minimise core component changes till they are refactored.
• Collaborators will query data and return analysis results.
• The aggregator will aggregate the analysis results.
• The Base Taskrunner class will be used for FA.
• Taskrunner Workspace: Calculate a histogram (an estimate of the probability distribution of a continuous variable) on specific columns of the Iris dataset.

Testing

[kminhta]

If this feature is at a stage where it needs a dummy model that is tied to a specific framework, I don't think we should introduce a dedicated task runner class outside of the workspace, yet.

[theakshaypant]

Agree with this! The current changes would require the users to install keras for running a federated analytics workspace which does not even use it.

[tanwarsh]

Moved this code to src folder and removed keras mnist model.

[payalcha]

@tanwarsh PR seems different from design #1385 in discussion. Can we update the design document as well or we can create a different design document in word or wiki.

[payalcha]

README.md document for this will be helpful as purpose of this is different from normal workspace.

[tanwarsh]

I will update documentation instead of adding README.md file.

[teoparvanov]

Thanks, @tanwarsh, I think it's looking better and more self-contained already! I especially like the fact that you've almost eliminated the changes to the core framework.

I have a couple of comments and suggestions from my first read-through.

PS: I see that the introduction of a dummy model introduces some complexity, for which I suggest an alternative to explore.

[teoparvanov]

As suggested previously by @kta-intel and @theakshaypant , I suggest moving the FederatedAnalyticsTaskRunner under the src package in the workspace. At least until we figure out a cleaner way of integrating the FA use case with the core framework.

[tanwarsh]

Moved FederatedAnalyticsTaskRunner to src folder in workspace.

[teoparvanov]

same here, let's move FederatedAnalyticsDataLoader under src in the example workspace, while the interfaces are still evolving

[tanwarsh]

removed FederatedAnalyticsDataLoader.

[teoparvanov]

Let's call this AggregateHistogram

[tanwarsh]

renamed to AggregateHistogram.

[teoparvanov]

nit - suggestion to rename analysis to analytics, to better align with the concept of Federated Analytics

[tanwarsh]

renamed analysis to analytics.

[teoparvanov]

Let's also rename the tag to analytics

[tanwarsh]

done.

[teoparvanov]

Same here - for symmetry, let's call this query_task

[tanwarsh]

I have renamed this to analytics_task instead of query_task. The idea is to not get confused between the query (the information requested which is histogram in this workspace) with query (a request for specific information or data from a database).

[teoparvanov]

By the way, where does the col_name parameter come from? I didn't see it in the FL plan task definition:

tasks :
  analysis:
    function : analysis
    aggregation_type: 
      template: src.histogram.Histogram

    kwargs   :
      columns: ['sepal length (cm)', 'sepal width (cm)']

[tanwarsh]

sorry for the confusion. col_name is actually collaborator name here. the doc string is updated.

[teoparvanov]

-> analytics_result

[tanwarsh]

done.

[teoparvanov]

Instead of dynamically deriving the weights from a model object, couldn't we just set static ones?
Same goes for _get_weights_names() and get_tensor_dict()

[tanwarsh]

removed model and added static tensors.

[teoparvanov]

If my suggestion from above works, wouldn't it even be possible to set self.model = None ?

[tanwarsh]

in order to make self.model = None work I had to change code mentioned below in openfl/protocols/utils.py.

if round_number is None:
# For Federated Analytics, we are using a placeholder model with no tensors
    round_number = 0

Alternative to this can be to handle this in _load_initial_tensors() in openfl/component/aggregator/aggregator.py where encounters error as round number is None.

let me know your thoughts on this.

[psfoley]

@tanwarsh Thanks for adding this. I like that you've added a Histogram aggregation function. I do think there's an opportunity to simplify some areas (remove dedicated analytics taskrunner, dataloader), and make use of callbacks for bespoke aggregator functionality. I see your intention of trying to limit what developers see in the example src directory, which is good, but if you remove the keras dependency I think you can cleanly trim this down to one file and inherit directly from the TaskRunner class.

[psfoley]

These changes should be avoided as it bloats the scope of the save model function. There's a great opportunity to refactor this bespoke federated analytics code as a callback (i.e. on_round_end).

[teoparvanov]

Great idea, @psfoley - this also bothered me during my review, but I wasn't sure what alternative to suggest to Shailesh... The callback approach would indeed do the job in this case!

[tanwarsh]

moved this code to on_round_end callback.

[psfoley]

These files should be deleted IMO. You can inherit directly from TaskRunner in the example. You can (and should) avoid the dependency on Keras. Part of the point of Federated Analytics is there is no machine learning model to be trained; you are just running a statistical query, etc. I expect you'll just need to return empty lists / dictionaries for get_tensor_dict, set_tensor_dict, get_tensorkeys_for_function, etc. to plug into the existing logic

[tanwarsh]

removed keras dependency and added code to return static/empty list for tensors.

[tanwarsh]

@tanwarsh Thanks for adding this. I like that you've added a Histogram aggregation function. I do think there's an opportunity to simplify some areas (remove dedicated analytics taskrunner, dataloader), and make use of callbacks for bespoke aggregator functionality. I see your intention of trying to limit what developers see in the example src directory, which is good, but if you remove the keras dependency I think you can cleanly trim this down to one file and inherit directly from the TaskRunner class.

Thank you for the detailed review.

I will summarise the changes done below as per comments by @psfoley @teoparvanov @theakshaypant @kta-intel.

1. Removed dedicated FA dataloader to simply workspace.
2. Removed keras model and used static/empty tensors to bypass core related changes (please review the changes in openfl/protocols/utils.py and in openfl/component/aggregator/aggregator.py in order to make this work).
3. Moved FederatedAnalyticsTaskRunner code to src folder in workspace. Although it is advised to remove this but IMO we should keep it as this separates the model related changes done to make FA work with openfl and these changes will be no longer required once we refactor the framework to enable it to work with both FL and FA. Once refactoring is done FederatedAnalyticsTaskRunner will be left with code required for FA which can be moved back to framework instead of keeping it as part of workspace. This will help in simplify what user should focus on which is analytics and query logic in analytics_task in taskrunner.py and query function in dataloader respectively.
4. Moved the code from save model function to FederateAnalyticsCallback. (please review the changes done in aggregator to register the callback as we want the framework to work weather it is FL or FA. If we want to only use this callback for FA we will have to reintroduce mode/config in plan which was done earlier with core component changes, this will help in registering the callback only for FA.)

[teoparvanov]

Looking good @tanwarsh, thanks for the hard work that has gone into this!
I have a couple of minor comments and suggestions:

[theakshaypant]

Minor comments.

[teoparvanov]

Thanks @tanwarsh, this is a great first iteration of the FA capability for OpenFL!

[MasterSkepticista]

Not quite understood what this aggregation function does. Please elaborate from a data scientist's perspective what aggregating histograms is mathematically like. Example optimizer description.

[tanwarsh]

added details in docstring.

[MasterSkepticista]

If nothing is passed, it must be an error, not an empty result, isn't it?

[tanwarsh]

done

[MasterSkepticista]

If the intent was to tell the user it is not applicable, it is better to raise an error or let the docstring mention the same

[tanwarsh]

done

[MasterSkepticista]

A reader needs to know why it is a dummy model and how setting it to None helps. Bonus points if we can link an issue/discussion or even this PR to provide additional context.

[tanwarsh]

added comment.