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

Quickstart #20

Merged
57 commits merged into from
Jun 25, 2022
Merged

Quickstart #20

57 commits merged into from
Jun 25, 2022

Conversation

ryanolson
Copy link
Contributor

No description provided.

@ryanolson ryanolson requested a review from a team as a code owner June 10, 2022 15:21
quickstart/src/sources.cpp Outdated Show resolved Hide resolved
quickstart/include/sources.hpp Outdated Show resolved Hide resolved
quickstart/examples/cpp/CMakeLists.txt Outdated Show resolved Hide resolved
quickstart/CMakeLists.txt Outdated Show resolved Hide resolved
quickstart/CMakeLists.txt Outdated Show resolved Hide resolved
quickstart/environment_cpp.yml Outdated Show resolved Hide resolved
quickstart/environment_cpp.yml Outdated Show resolved Hide resolved
quickstart/include/sources.hpp Outdated Show resolved Hide resolved
quickstart/examples/cpp/00_Quickstart/quickstart.cpp Outdated Show resolved Hide resolved
quickstart/examples/cpp/00_Quickstart/quickstart.cpp Outdated Show resolved Hide resolved
@cwharris cwharris added improvement Improvement to existing functionality non-breaking Non-breaking change labels Jun 10, 2022
Copy link
Contributor Author

@ryanolson ryanolson left a comment

Choose a reason for hiding this comment

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

thanks @cwharris

@ryanolson ryanolson changed the base branch from main to branch-22.06 June 10, 2022 16:44
@ryanolson ryanolson changed the base branch from branch-22.06 to main June 10, 2022 16:48
Copy link
Contributor

@mdemoret-nv mdemoret-nv 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 we need to sync on a few things:

  1. Organization
    1. I like the 00_Name format but the first one shouldnt be called "Quickstart". Are we only going to have one example (And the number isnt necessary)?
  2. How much the python and C++ examples will mirror each other
    3. The way things are done in python and C++ is a bit different. We could have line level parity if we wanted to
  3. What exactly we want to show
    1. The first example should be easy and then build up. But it doesnt look like that was the plan from your first pass. Do you intend to have more examples?

quickstart/examples/cpp/00_Quickstart/quickstart.cpp Outdated Show resolved Hide resolved
quickstart/examples/cpp/00_Quickstart/quickstart.cpp Outdated Show resolved Hide resolved
quickstart/examples/cpp/00_Quickstart/quickstart.cpp Outdated Show resolved Hide resolved
quickstart/README.md Outdated Show resolved Hide resolved
quickstart/examples/python/00_SimplePipeline/README.md Outdated Show resolved Hide resolved
quickstart/README.md Outdated Show resolved Hide resolved
quickstart/examples/cpp/00_Quickstart/quickstart.cpp Outdated Show resolved Hide resolved
@ryanolson ryanolson changed the base branch from main to branch-22.06 June 14, 2022 23:22
@ryanolson ryanolson changed the title C++ Quickstart Quickstart Jun 16, 2022
@mdemoret-nv
Copy link
Contributor

I would like to get an example of #42 into the quickstart guide, but it can wait until 22.08.

I think post 22.08 will be the best time to include that.

Copy link
Contributor

@drobison00 drobison00 left a comment

Choose a reason for hiding this comment

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

Looking really good overall. Added a few comments + suggestion.

docs/quickstart/README.md Outdated Show resolved Hide resolved
docs/quickstart/README.md Outdated Show resolved Hide resolved
docs/quickstart/README.md Outdated Show resolved Hide resolved
docs/quickstart/README.md Outdated Show resolved Hide resolved
docs/quickstart/README.md Outdated Show resolved Hide resolved
python/CMakeLists.txt Outdated Show resolved Hide resolved
python/srf/benchmarking/CMakeLists.txt Outdated Show resolved Hide resolved
python/srf/core/CMakeLists.txt Outdated Show resolved Hide resolved
python/srf/core/CMakeLists.txt Outdated Show resolved Hide resolved
python/srf/tests/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link

@BartleyR BartleyR left a comment

Choose a reason for hiding this comment

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

Overall the QSG is good and a very nice v1. We can iterate on it going forward. None of the changes I mentioned are mandatory, so approving this as-is. I couldn't find two of the README files for the C++ examples (perhaps I overlooked them), so we'd want to address that in short order if it's indeed an issue. Also, there are some minor nits in the example READMEs about the benefit of including 1-2 sentences at the end explaining the output and what's happening to the user. Overall, this is solid work.

docs/quickstart/README.md Outdated Show resolved Hide resolved
docs/quickstart/README.md Outdated Show resolved Hide resolved
docs/quickstart/README.md Outdated Show resolved Hide resolved
docs/quickstart/README.md Show resolved Hide resolved
Copy link
Contributor

@drobison00 drobison00 left a comment

Choose a reason for hiding this comment

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

LGTM.

fix broken link
Copy link
Contributor Author

@ryanolson ryanolson left a comment

Choose a reason for hiding this comment

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

I can't request changes on my own PR...

docs/quickstart/README.md Outdated Show resolved Hide resolved
docs/quickstart/README.md Outdated Show resolved Hide resolved
docs/quickstart/README.md Outdated Show resolved Hide resolved
docs/quickstart/README.md Outdated Show resolved Hide resolved
docs/quickstart/README.md Outdated Show resolved Hide resolved
docs/quickstart/README.md Outdated Show resolved Hide resolved
docs/quickstart/README.md Outdated Show resolved Hide resolved
docs/quickstart/README.md Outdated Show resolved Hide resolved
@mdemoret-nv
Copy link
Contributor

@gpucibot merge

@ghost ghost merged commit cd2e51a into nv-morpheus:branch-22.06 Jun 25, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement to existing functionality non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants