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.

Sign up for GitHub

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

Jump to bottom

Doc cleanup for nx-cugraph: fixed typos, cleaned up various descriptions, renamed notebook to match naming convetion. #4478

Merged
merged 11 commits into from
Jul 2, 2024
Merged

Doc cleanup for nx-cugraph: fixed typos, cleaned up various descriptions, renamed notebook to match naming convetion. #4478

merged 11 commits into from
Jul 2, 2024

Conversation

rlratzel
Copy link
Contributor

closes #4466

  • Fixed typos
  • Changed various descriptions to match existing terminology and hopefully clarify
  • Renamed notebook to match file name conventions

@rlratzel
Doc cleanup for nx-cugraph: fixed typos, cleaned up various descripti…
ce6ed4c 
…ons, renamed notebook to match naming convetion.
@rlratzel rlratzel added doc Documentation non-breaking Non-breaking change labels Jun 10, 2024
@rlratzel rlratzel added this to the 24.08 milestone Jun 10, 2024
@rlratzel rlratzel self-assigned this Jun 10, 2024
@rlratzel rlratzel requested a review from a team as a code owner June 10, 2024 18:36
@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@rlratzel rlratzel mentioned this pull request Jun 10, 2024
Closed
2 tasks
acostadon
acostadon approved these changes Jun 10, 2024
View reviewed changes
Copy link
Contributor

@acostadon acostadon left a comment

Choose a reason for hiding this comment

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

Your wording is terrific. Thanks for going over it.

eriknw
eriknw reviewed Jun 11, 2024
View reviewed changes
Copy link
Contributor

@eriknw eriknw left a comment

Choose a reason for hiding this comment

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

Thanks for jumping on this @rlratzel, these changes are an improvement. I found some typos.

docs/cugraph/source/nx_cugraph/nx_cugraph.md Outdated Show resolved Hide resolved
docs/cugraph/source/nx_cugraph/nx_cugraph.md Outdated Show resolved Hide resolved
docs/cugraph/source/nx_cugraph/nx_cugraph.md Outdated Show resolved Hide resolved
docs/cugraph/source/nx_cugraph/nx_cugraph.md Outdated Show resolved Hide resolved
docs/cugraph/source/nx_cugraph/nx_cugraph.md Outdated Show resolved Hide resolved
notebooks/cugraph_benchmarks/nx_cugraph_benchmark.ipynb Outdated Show resolved Hide resolved
acostadon
acostadon approved these changes Jun 24, 2024
View reviewed changes
Copy link
Contributor

@acostadon acostadon left a comment

Choose a reason for hiding this comment

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

Looks good

@rlratzel rlratzel requested a review from a team as a code owner July 1, 2024 17:55
@rlratzel rlratzel requested a review from jameslamb July 1, 2024 17:55
eriknw
eriknw requested changes Jul 1, 2024
View reviewed changes
Copy link
Contributor

@eriknw eriknw 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 this is very close. I have minor formatting nits.

First, I think more things should be hyphenated:

  • command line -> command-line
  • run time -> run-time
  • preconvert -> pre-convert

Second, I think we could link to networkx algos:

Third, is the reference to GTC Spring 2024 supposed to link to something? I'm not sure why it's mentioned if not.

Fourth, why does run_algos use nx.bfs_tree but the message in the notebook says bfs_edges? Is this a typo, or is there something weird going on?

Fifth, the code in the notebook cells could be formatted more nicely. For example, adding spaces between = and +.

notebooks/cugraph_benchmarks/nx_cugraph_benchmark.ipynb Outdated Show resolved Hide resolved
jameslamb
jameslamb approved these changes Jul 1, 2024
View reviewed changes
Copy link
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Packaging changes look OK to me. Hopefully thriftpy2==0.5.2 will be release soon and you'll be able to switch this <= to a !=: Thriftpy/thriftpy2#281 (comment)

rlratzel added 2 commits July 1, 2024 22:27
@rlratzel
Refactors notebook for clarity: cleans up code cells, more changes fo…
173bac3 
…r consistency, includes cell outputs to show benchmark comparison, rewrites explanations in markdown cells.
@rlratzel
Merge remote-tracking branch 'upstream/branch-24.08' into branch-24.0…
7858f6b 
…8-nxcg_doc_fix
eriknw
eriknw approved these changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

@eriknw eriknw left a comment

Choose a reason for hiding this comment

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

I'm looking forward to getting networkx/networkx#7497 in so we can make the warnings less noisy.

@rlratzel
Copy link
Contributor Author

rlratzel commented Jul 2, 2024

/merge

@rapids-bot rapids-bot bot merged commit eab0460 into rapidsai:branch-24.08 Jul 2, 2024
137 checks passed
@aisk
Copy link

aisk commented Jul 5, 2024 •
edited
Loading

Packaging changes look OK to me. Hopefully thriftpy2==0.5.2 will be release soon and you'll be able to switch this <= to a !=: Thriftpy/thriftpy2#281 (comment)

Hi, thriftpy2 v0.5.2 has just been released. And I will release a beta version before every thriftpy2 release to avoid breaks in the future. And if anyone has a test environment and want to test the beta release, they can use pip install --pre ... to install the pre-release version of the dependencies and help find the bugs.

@jameslamb jameslamb mentioned this pull request Jul 5, 2024
Merged
@jameslamb
Copy link
Member

Thanks very much @aisk ! I've put up #4521 to test cugraph with v0.5.2.

rapids-bot bot pushed a commit that referenced this pull request Jul 19, 2024
@jameslamb
remove thriftpy2 ceiling (#4521)
c9d2843 
#4496 introduced a ceiling on `thriftpy2`. Context: #4496 (comment)

The bug that ceiling was added to avoid was fixed in v0.5.2 of `thriftpy2`, which was just released (#4478 (comment)).

This removes that, adding `!=` constraints to skip the 2 versions that `cugraph` was not compatible with.

## Notes for Reviewers

### Why not a floor?

I'm proposing adding `!=` constraints to skip v0.5.0 and v0.5.1 to maximize `cugraph`'s compatibility with other environments... that'd allow it to be used in environments with `thriftpy2<0.5.0` and in environments with `thriftpy2>0.5.2`.

Let me know if you'd prefer the simplicity of a floor like `>=0.5.2` instead.

Authors:
  - James Lamb (https://github.com/jameslamb)
  - Ralph Liu (https://github.com/nv-rliu)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Ralph Liu (https://github.com/nv-rliu)
  - Rick Ratzel (https://github.com/rlratzel)

URL: #4521
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acostadon acostadon acostadon approved these changes

@jameslamb jameslamb jameslamb approved these changes

@eriknw eriknw eriknw approved these changes

Assignees

@rlratzel rlratzel

Labels
doc Documentation non-breaking Non-breaking change python
Projects
None yet
Milestone
24.08
Development

Successfully merging this pull request may close these issues.

[BUG]: Nx-cuGraph caching not working with documentation example
6 participants
@rlratzel @aisk @jameslamb @eriknw @acostadon @nv-rliu