Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding Module Dependencies feature #1876

Merged
merged 22 commits into from
Dec 11, 2023
Merged

Adding Module Dependencies feature #1876

merged 22 commits into from
Dec 11, 2023

Conversation

kessler-frost
Copy link
Member

@kessler-frost kessler-frost commented Dec 6, 2023

  • I have added the tests to cover my changes.
  • I have updated the documentation and CHANGELOG accordingly.
  • I have read the CONTRIBUTING document.

Copy link

codecov bot commented Dec 6, 2023

Codecov Report

Merging #1876 (90cfae9) into develop (d94bdaf) will increase coverage by 0.45%.
The diff coverage is 95.29%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1876      +/-   ##
===========================================
+ Coverage    84.43%   84.88%   +0.45%     
===========================================
  Files          294      181     -113     
  Lines        14389    11184    -3205     
  Branches       195        0     -195     
===========================================
- Hits         12149     9494    -2655     
+ Misses        2106     1690     -416     
+ Partials       134        0     -134     
Flag Coverage Δ *Carryforward flag
Dispatcher 92.14% <ø> (-0.19%) ⬇️ Carriedforward from 9483dc7
Functional_Tests ?
SDK 79.47% <95.29%> (+0.16%) ⬆️
UI_Backend ?
UI_Frontend ?

*This pull request uses carry forward flags. Click here to find out more.

@santoshkumarradha
Copy link
Member

@kessler-frost reminder on the usual list of docs changes for this as well along with this PR

CC: @sriranjanivenkatesan once the docs are up, please keep a tab to update it

@kessler-frost kessler-frost marked this pull request as ready for review December 11, 2023 12:41
@kessler-frost kessler-frost requested a review from a team as a code owner December 11, 2023 12:41
@kessler-frost
Copy link
Member Author

One thing to note here is that DepsModule are kind of a fake traditional “deps” in the sense that we don’t require them to be passed to the server as cloudpickle takes care of serializing the module when the serialization of the function happens.

@kessler-frost
Copy link
Member Author

One drawback of this is that there we won't be recording the name of the module in the dispatcher server. @santoshkumarradha it's a UX question as in do we want to have a record or not? This is mostly an additive change and can be taken care of in a separate PR as well.

Copy link
Member

@santoshkumarradha santoshkumarradha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UX is gtg 🤘

@santoshkumarradha
Copy link
Member

One drawback of this is that there we won't be recording the name of the module in the dispatcher server. @santoshkumarradha it's a UX question as in do we want to have a record or not? This is mostly an additive change and can be taken care of in a separate PR as well.

Thanks @kessler-frost , the only reason would be to metadata add it in and error propagation, but lack of proper pickling should already through that error from cloud pickle side, so we are good on that front. We can backlog a PR for storing the metadata about moduledeps later this is gtg :).

@cjao cjao merged commit 6e331d4 into develop Dec 11, 2023
14 checks passed
@cjao cjao deleted the module-deps branch December 11, 2023 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants