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

feat: architecture armonik extension development #46

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

italo1aguiar-aneo
Copy link

No description provided.

@CLAassistant
Copy link

CLAassistant commented May 31, 2024

CLA assistant check
All committers have signed the CLA.

AEP/aep-00004.md Outdated Show resolved Hide resolved
Copy link
Contributor

@aneojgurhem aneojgurhem left a comment

Choose a reason for hiding this comment

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

This AEP is a bit short compared to all the sections from our list of sections. As it describes our high level sdk, it is not really linked to our model.
I expect some of the following subjects to be addressed:

  • Not rely too much on C#. (especially some namings)
  • Some examples to show how it is expected to be used. How do I define a task ? How am I expected to get my output ?
  • What is a task ? a result ?
  • ResultService should not appear as we want to rename to BlobService or some other thing with a clearer meaning.
  • How do I monitor things ? What can I know and how ?
  • Worker side has not been adressed yet (I now it is not for now) but I would expect a few words about it.
  • SDKs should provide ways to easily define ArmoniK based applications (client and worker side). I think this should appear in the abstract and motivation parts. Why do we want that should appear too.

AEP/aep-00004.md Outdated

This AEP aims to create a standard architecture for ArmoniK's extension development. It proposes services decoupling, segregating interfaces to related domains, and enriching domains.

# Proposed Solution
Copy link
Contributor

Choose a reason for hiding this comment

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

Proposed Solution is not a section available in our template. See AEP1.

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