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

rename clientside components, add client for server communication #467

Merged
merged 15 commits into from
Dec 13, 2024

Conversation

eelcovdw
Copy link
Contributor

@eelcovdw eelcovdw commented Dec 5, 2024

Description

NOTE: sync plugin contains some changes from #397 . Merging this PR before #397 will make a small conflict in how we validate permissions in sync_action.py, for the rest they are compatible

Renames:

  • client2.py -> core.py
  • SyftClient -> SyftBoxRunner
    • Note: we dont use client here, because then the naming of actual clients is confusing.
  • Plugins -> PluginManagerInterface
  • SyftClientInterface -> SyftBoxContextInterface
  • In plugins: var client is now called context

New:

  • Move server API endpoints to SyftBoxClient(ClientBase)
  • add to LocalSyftBox.client: SyftBoxClient and Context.client: SyftBoxClient
  • different APIs are added to different namespaces in SyftBoxClient:
class SyncClient(ClientBase):
    def download(self, path: Path) -> bytes: ...

class SyftBoxClient(ClientBase):
    def __init__(self, conn: httpx.Client):
        self.conn = conn
        self.sync = SyncClient(conn)

# usage
client = SyftBoxClient.from_config(my_syftbox_config)
client.sync.download(Path("my_file"))

Affected Dependencies

List any dependencies that are required for this change.

How has this been tested?

  • Describe the tests that you ran to verify your changes.
  • Provide instructions so we can reproduce.
  • List any relevant details for your test configuration.

Checklist

@eelcovdw eelcovdw changed the title [WIP] rename clientside components, add client for server communication rename clientside components, add client for server communication Dec 11, 2024
Copy link
Contributor

@koenvanderveen koenvanderveen left a comment

Choose a reason for hiding this comment

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

I think in general there are two things that I have bigger questions about.

  1. why does context have a client?
  2. wouldnt it be better to pass a LocalSyftbox instance around instead of a context

@eelcovdw
Copy link
Contributor Author

I think in general there are two things that I have bigger questions about.

  1. why does context have a client?
  2. wouldnt it be better to pass a LocalSyftbox instance around instead of a context

Discussed this in call, recap:

  • The logic between Context and SyftBoxRunner is a bit double, causing some code duplication. Renamed the main syftbox class to SyftBoxRunner to make this clearer, but we might want to revisit the structures and interfaces in the future
  • Context.client is just there for convenience, plugins could also manage their own client (in plugin: SyftBoxClient.from_context(ctx)). Leaving it as-is for now, can make this change in the future if we like it better

@eelcovdw eelcovdw merged commit 12a0970 into main Dec 13, 2024
14 checks passed
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.

2 participants