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

docs: hello world readme #32

Closed
wants to merge 57 commits into from
Closed

docs: hello world readme #32

wants to merge 57 commits into from

Conversation

nnshah1
Copy link
Contributor

@nnshah1 nnshah1 commented Jan 17, 2025

What does the PR do?

MVP documentation for running hello world example.

Checklist

  • PR title reflects the change and is of format <commit_type>: <Title>
  • Changes are described in the pull request.
  • Related issues are referenced.
  • Populated github labels field
  • Added test plan and verified test passes.
  • Verified that the PR passes existing CI.
  • Verified copyright is correct on all changed files.
  • Added succinct git squash message before merging ref.
  • All template sections are filled out.
  • Optional: Additional screenshots for behavior/output changes with before/after.

Commit Type:

Check the conventional commit type
box here and add the label to the github PR.

  • build
  • ci
  • docs
  • feat
  • fix
  • perf
  • refactor
  • revert
  • style
  • test

Related PRs:

Where should the reviewer start?

Test plan:

  • CI Pipeline ID:

Caveats:

Background

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

  • closes GitHub issue: #xxx

@nnshah1 nnshah1 requested review from tanmayv25 and rmccorm4 January 17, 2025 19:40
@nnshah1 nnshah1 changed the base branch from main to nnshah1-hello-world January 17, 2025 19:40
README.md Outdated Show resolved Hide resolved
examples/hello_world/README.md Outdated Show resolved Hide resolved
examples/hello_world/README.md Outdated Show resolved Hide resolved
examples/hello_world/README.md Outdated Show resolved Hide resolved
examples/hello_world/README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Comment on lines 215 to 220
#### Standalone Operator

The encoder-decoder operator is a standalone python class that
implements the Operator interface. Internally it makes remote requests
to other workers. Generally a standalone operator can make use of
other operators for its work but isn't required to.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should "Standalone" be more like "Custom"? Custom operator that you can implement to do what you want? In this case, it just happened to be encoder-decoder, but it could be anything and has to be implemented by the user.

Compared to TritonCoreOperator which is a built-in/pre-defined operator that can be imported from the library out of the box.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure - any other thoughts ... custom operator seemed a lot like custom model .... and I think its a usual case. custom some how implies and outlier to me - like you only need it sometimes .... maybe its just Operator? or Generic Operator?

Copy link
Contributor

Choose a reason for hiding this comment

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

We generally have used the term "custom" to describe the scenario where we've defined an interface for you that you can implement/use to do your own things, ex:

  • custom backend
  • custom metrics
  • custom tracing
  • etc

Copy link
Contributor

Choose a reason for hiding this comment

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

But don't mind whatever you go with for now - can be tuned

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer custom operator as well.

Copy link
Contributor

@rmccorm4 rmccorm4 left a comment

Choose a reason for hiding this comment

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

Generally LGTM - needs merge conflicts resolved

Base automatically changed from nnshah1-hello-world to main January 17, 2025 23:57
@nnshah1 nnshah1 closed this Jan 19, 2025
@nnshah1
Copy link
Contributor Author

nnshah1 commented Jan 19, 2025

merged seperately

@nnshah1 nnshah1 deleted the nnshah1-hello-world-readme branch January 19, 2025 16:44
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.

4 participants