[Feature] Add BasicClientProtocol and AdaptiveDriftConstrainedMixin

[nerdai]

PR Type

Feature

Short Description

NOTE
This is a split of #364 which got a bit larger than expected. So splitting it into more manageable bits for ease of review.

This PR adds a mixin version of AdaptiveDriftConstrainedMixin to promote composability rather than inheritance. This is a pre-cursor PR to the ditto mixin which will subclass the this adaptive one.

Add a short description of what is in this PR.

Tests Added

• added tests for mixin

[codecov]

Codecov Report

Attention: Patch coverage is 88.79310% with 13 lines in your changes missing coverage. Please review.

Project coverage is 76.15%. Comparing base (9338be2) to head (a175745).
Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
fl4health/mixins/adaptive_drift_constrained.py	83.95%	13 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #384      +/-   ##
==========================================
+ Coverage   75.99%   76.15%   +0.16%     
==========================================
  Files         145      148       +3     
  Lines        8809     8924     +115     
==========================================
+ Hits         6694     6796     +102     
- Misses       2115     2128      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[emersodb]

Overall, I think this looks pretty good. Most of my questions are likely due to my lack of expertise in mixins. So please forgive some of the questions if they are naive.

[emersodb]

This is sort of a hard line to follow. I actually had to throw this through the debugger to get what was happening. Correct me if I'm wrong, but you're actually instantiating an object to extract its type here by using a string to construct the class name?

This might be the best way to do it (I haven't thought about it at all 😂) but thought I'd at least suggest taking two seconds to verify that it is ha. It sort of feels funny to me just because of the indirection.

[nerdai]

I'm actually just creating a new class dynamically here. The thinking here is to provide a convenience method (or syntactic sugar) for the user to quickly adapt their clients with the mixins we support.

I've added a test test_dynamically_created_class that tests its usage. But basically

from fl4health.clients import BasicClient

class MyBasicClient(BasicClient):
    ...

# instead of creating a new class manually that mixes `MyBasicClient` with
# this `AdaptiveDriftConstrainedMixin`, we have this syntactic sugar to do it for 
# the user that returns the mixed class/type.

my_adapted_class = apply_adaptive_drift_to_client(MyBasicClient)

# this is equivalent to the following (with the exception of the missing `_dynamically_created` cls attribute)
class MyAdaptedClass(AdaptiveDriftConstrainedMixin, MyBasicClient):
    pass

[emersodb]

It's a bit clearer now why you want to do this. It's still a difficult line to read/understand (the black formatting probably isn't helping ha). I'm fine with it as sugar.

[nerdai]

Overall, I think this looks pretty good. Most of my questions are likely due to my lack of expertise in mixins. So please forgive some of the questions if they are naive.

All good. I actually think this scenario helps at times in getting good code reviews.

[emersodb]

I think you're going to get a linter error here for the f-string without a variable?

[nerdai]

For some reason, this passed our lint checks, but thanks for catching this. I've removed the f-string annotation.

[emersodb]

Good to go.

[nerdai]

Thanks for the reviews, @emersodb -- landing this PR now.