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

Remove nx-cugraph from cugraph #418

Merged

Conversation

eriknw
Copy link
Contributor

@eriknw eriknw commented Nov 14, 2024

I believe this is needed to remove nx-cugraph from the cugraph repo being done in rapidsai/cugraph#4756

This is a subset of the changes @jameslamb is making in #417. Does it make sense to do this change for nx-cugraph and cugraph-gnn independently (how close is cugraph-gnn to being removed?)? Should we also add nx-cugraph as done in that PR? Anything else?

@nv-rliu @jameslamb @rlratzel

@eriknw eriknw requested a review from a team as a code owner November 14, 2024 02:23
@eriknw eriknw requested review from msarahan and removed request for a team November 14, 2024 02:23
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.

This is not the right fix. Don't you still want nx-cugraph to be buildable with devcontainers? Even if we don't add devcontainer CI / devcontainer configs in the https://github.com/rapidsai/nx-cugraph repo, I think it'd still be useful to have that support here in the unified devcontainer... so you could make a change in cugraph and then test how it affects nx-cugraph.

That is why in #417, I added something like this:

- name: nx-cugraph
  path: nx-cugraph
  git: {<<: *git_defaults, repo: nx-cugraph}
  python:
    - name: nx-cugraph
      sub_dir: .
      args: {install: *rapids_build_backend_args}

And please see the other change there... any time you change the content of something in features/ here, you need to bump it's version.

image

Once those are addressed, I support moving this PR forward and not trying to wait on the GNN projects. I'd hoped to be able to do this all at once, but cugraph-gnn needs a bit more work.

@jameslamb jameslamb removed the request for review from msarahan November 14, 2024 14:44
@jameslamb
Copy link
Member

Also note that the CI failures here weren't because of your changes, and should resolve on a re-run now that rapidsai/raft#2490 was merged.

@eriknw
Copy link
Contributor Author

eriknw commented Nov 14, 2024

Thanks @jameslamb I suspected we needed to make those changes, thanks for explaining :) . Updated.

@trxcllnt
Copy link
Collaborator

@jameslamb Even if we don't add devcontainer CI / devcontainer configs in the nx-cugraph repo

We should add devcontainers to the new nx-cugraph repo. Aside from them being a convenient way to work on a project for outside contributors, they also help ensure nx-cugraph changes don't break devcontainer and DLFW builds.

@jameslamb
Copy link
Member

We should add devcontainers to the new nx-cugraph repo

Great, let's do that. Want to put up a PR at https://github.com/rapidsai/nx-cugraph?

@jameslamb
Copy link
Member

jameslamb commented Nov 14, 2024

Believe CI errors like this:

/home/coder/cuvs/cpp/src/neighbors/iface/iface.hpp(176): error: incomplete type is not allowed

1 error detected in the compilation of "/home/coder/cuvs/cpp/src/neighbors/iface/iface_cagra_float_uint32_t.cu".

(build link)

Should be fixed by rapidsai/cuvs#467

@jameslamb
Copy link
Member

I put up a PR to add devcontainers CI jobs for nx-cugraph: rapidsai/nx-cugraph#25

@jameslamb jameslamb self-requested a review November 15, 2024 05:01
@jameslamb jameslamb added breaking Introduces a breaking change improvement Improves an existing functionality labels Nov 15, 2024
@jameslamb
Copy link
Member

Even though this change is also covered in #417, CI just started over there and takes a couple hours to run.

Merging this to unblock rapidsai/cugraph#4756 and rapidsai/nx-cugraph#25

@jameslamb jameslamb merged commit 7b72a8d into rapidsai:branch-24.12 Nov 15, 2024
26 checks passed
bdice pushed a commit that referenced this pull request Nov 19, 2024
Development of some `cugraph` projects is moving in 24.12.

```text
# GNN packages
cugraph-dgl: rapidsai/cugraph -> rapidsai/cugraph-gnn
cugraph-pyg: rapidsai/cugraph -> rapidsai/cugraph-gnn
wholegraph: rapidsai/wholegraph -> rapidsai/cugraph-gnn

# networkx
nx-cugraph: rapidsai/cugraph -> rapidsai/nx-cugraph

# other
cugraph-equivariant -> (removed)
```

This updates the `rapids-build-utils` manifest to reflect those changes.

## Notes for Reviewers

The `nx-cugraph` changes ended up getting split off into their own PR:
#418

---------

Co-authored-by: Kyle Edwards <[email protected]>
rapids-bot bot pushed a commit to rapidsai/nx-cugraph that referenced this pull request Nov 20, 2024
Adds devcontainers and devcontainer CI jobs.

## Notes for Reviewers

I created this by copying from the `cugraph` repo and then just changing all the references to `nx-cugraph` (and removing a few unnecessary things, like details about cloning cugraph-ops).

This will be blocked until rapidsai/devcontainers#417 or rapidsai/devcontainers#418 is merged.

Authors:
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - Erik Welch (https://github.com/eriknw)
  - Paul Taylor (https://github.com/trxcllnt)
  - Ralph Liu (https://github.com/nv-rliu)
  - Bradley Dice (https://github.com/bdice)

URL: #25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Introduces a breaking change improvement Improves an existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants