search

openscilab / pymilo

PyMilo: Python for ML I/O
MIT License
148 stars 5 forks source link

Feature/ml streaming base #115

Closed AHReccese closed 3 months ago

AHReccese commented 3 months ago

Reference Issues/PRs

The base architecture of ML Streaming

Any other comments?

AHReccese commented 3 months ago

@sepandhaghighi I will prepare and drop a video in 24 hours in order to get onboarded better

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 55.00000% with 81 lines in your changes missing coverage. Please review.

Project coverage is 82.94%. Comparing base (06716ac) to head (6ca69a9). Report is 23 commits behind head on dev.

Files Patch % Lines
pymilo/streaming/communicator.py 36.51% 40 Missing :warning:
pymilo/streaming/pymilo_server.py 0.00% 30 Missing :warning:
pymilo/streaming/pymilo_client.py 81.25% 4 Missing and 5 partials :warning:
pymilo/streaming/compressor.py 87.50% 1 Missing :warning:
pymilo/streaming/encryptor.py 87.50% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #115 +/- ## ========================================== - Coverage 87.40% 82.94% -4.45% ========================================== Files 31 42 +11 Lines 1142 1945 +803 Branches 201 394 +193 ========================================== + Hits 998 1613 +615 - Misses 90 232 +142 - Partials 54 100 +46 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

AHReccese commented 3 months ago

Oh god finally tests are executing successfully, I ran into some weird hard-to-debug issues, hopefully, they all are solved now. Continue with your review :) @sepandhaghighi @sadrasabouri

AHReccese commented 3 months ago

OK, sure. I will address them all in the next PRs. @sepandhaghighi

AHReccese commented 3 months ago

@AHReccese Thanks for your efforts 🔥

This pull request is a base PR and looks good to me, but I have some comments that should be addressed in the next PRs:

  1. I believe that we should add pydantic to both streaming-requirements.txt and dev-requirements.txt.
  2. We should separate streaming tests from core tests(Test the core features of PyMilo without installing streaming dependencies, and then install and test the streaming features.)
  3. Update all docstrings
  4. I suggest using enum instead of string for parameters like mode (for example ClientMode.Local instead of LOCAL).

1,3,4 are resolved.