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

Use conda to build python packages during GPU tests #2230

Merged
merged 1 commit into from
May 12, 2022

Conversation

jjacobelli
Copy link
Contributor

@jjacobelli jjacobelli commented Apr 15, 2022

This PR convert the from sources build we are doing in GPU test job to a conda build. This is done for the following reasons:

  • This is required step to improve the Ops CI/CD setup to a more convenient pipeline
  • This is required to start using conda compilers and mamba to build RAPIDS packages
  • This prevent us from manually managing and installing the dependencies in GPU job
  • This ensure the packages can be installed
  • This ensure the tests are running and working against the package content and not the build results. Currently the Python packages are not tested.

This may increase the global pipeline time, but the usage of mamba should resolve this as mamba is faster than conda to build packages

@jjacobelli jjacobelli added improvement Improvement / enhancement to an existing function DO NOT MERGE Hold off on merging; see PR for details non-breaking Non-breaking change labels Apr 15, 2022
@jjacobelli jjacobelli self-assigned this Apr 15, 2022
@jjacobelli jjacobelli requested a review from a team as a code owner April 15, 2022 09:09
@jjacobelli jjacobelli marked this pull request as draft April 15, 2022 09:45
@jjacobelli jjacobelli force-pushed the remove-from-soures branch 2 times, most recently from 3e2224d to eb6211d Compare April 21, 2022 08:21
@jjacobelli
Copy link
Contributor Author

rerun tests

@jjacobelli
Copy link
Contributor Author

rerun tests

@jjacobelli jjacobelli removed the DO NOT MERGE Hold off on merging; see PR for details label May 4, 2022
@codecov-commenter
Copy link

codecov-commenter commented May 4, 2022

Codecov Report

Merging #2230 (a87fe2e) into branch-22.06 (38be932) will decrease coverage by 6.66%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##           branch-22.06    #2230      +/-   ##
================================================
- Coverage         70.82%   64.15%   -6.67%     
================================================
  Files               170       99      -71     
  Lines             11036     4408    -6628     
================================================
- Hits               7816     2828    -4988     
+ Misses             3220     1580    -1640     
Impacted Files Coverage Δ
python/cugraph/cugraph/__init__.py 100.00% <ø> (ø)
python/cugraph/cugraph/centrality/__init__.py 100.00% <ø> (ø)
...graph/cugraph/centrality/betweenness_centrality.py 89.65% <ø> (ø)
...thon/cugraph/cugraph/centrality/katz_centrality.py 88.23% <ø> (-1.24%) ⬇️
python/cugraph/cugraph/community/egonet.py 97.36% <ø> (ø)
...ython/cugraph/cugraph/community/ktruss_subgraph.py 85.29% <ø> (ø)
...cugraph/cugraph/dask/centrality/katz_centrality.py 25.00% <ø> (ø)
python/cugraph/cugraph/dask/common/input_utils.py 22.13% <ø> (+0.57%) ⬆️
python/cugraph/cugraph/dask/common/mg_utils.py 30.43% <ø> (-7.07%) ⬇️
python/cugraph/cugraph/dask/common/part_utils.py 19.23% <ø> (-0.94%) ⬇️
... and 121 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 081df0a...a87fe2e. Read the comment docs.

@jjacobelli jjacobelli force-pushed the remove-from-soures branch 2 times, most recently from dd07e4f to 2a069a7 Compare May 5, 2022 08:54
@jjacobelli
Copy link
Contributor Author

rerun tests

@BradReesWork BradReesWork added this to the 22.06 milestone May 5, 2022
@jjacobelli jjacobelli force-pushed the remove-from-soures branch 2 times, most recently from 93a3d4d to dccbc3f Compare May 5, 2022 15:46
@jjacobelli jjacobelli marked this pull request as ready for review May 5, 2022 19:01
@jjacobelli jjacobelli requested a review from a team as a code owner May 5, 2022 19:01
@jjacobelli jjacobelli marked this pull request as draft May 5, 2022 19:01
@jjacobelli jjacobelli force-pushed the remove-from-soures branch 2 times, most recently from d4e4672 to a87fe2e Compare May 6, 2022 11:50
@jjacobelli jjacobelli marked this pull request as ready for review May 6, 2022 14:15
Copy link
Member

@ajschmidt8 ajschmidt8 left a comment

Choose a reason for hiding this comment

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

Approving ops-codeowner file changes.

Copy link
Contributor

@rlratzel rlratzel left a comment

Choose a reason for hiding this comment

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

Most of the changes look okay, but I did see a few that could cause problems which I commented on below. I also don't understand why some of these changes are needed for allowing the GPU tests to use a conda install instead of a from-source build (eg. why can't we continue to have a cugraph.tests package? Why do we need a separate cugraph.testing package?, etc.). Maybe we can chat offline about it and then update the PR or report back here, if necessary?

Comment on lines 24 to 27
# As tests directory is not a module, we need to add it to the path
sys.path.insert(0, '../')
Copy link
Contributor

Choose a reason for hiding this comment

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

A few things:

  • this does not add the tests directory to the import path, it adds .., which will be interpreted as the parent directory of the CWD from where pytest is run.
  • pytest automatically adds the directories containing the collected test modules to the import path, so this may be "accidentally" working because of that
  • these changes are not being tested by CI since our test script skips everything in the mg directory, so have you manually tested these changes on a multi-GPU machine to ensure they work?
  • needing to modify sys.path directly could be a symptom that we have a broken package hierarchy or some other code smell - is there a reason this directory is no longer a package (ie. had its __init__.py file removed)?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Ethyling based on our conversation, would you mind removing the sys.path.insert() calls and adding a FIXME stating that those tests need to be refactored to not import other tests and should instead put shared test code in a util module? Same for the other comment below.

Comment on lines 25 to 28
# As tests directory is not a module, we need to add it to the path
sys.path.insert(0, '.')
Copy link
Contributor

Choose a reason for hiding this comment

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

(same feedback as above)

@rlratzel
Copy link
Contributor

@Ethyling and I just had a call about this and we agreed on the following:

  • The changes to remove the __init__.py file so cugraph.tests is no longer a package is to force pytests to find imported modules from installs rather than relative to the source tree where the tests are located. pytest by default will walk the test location tree upwards until it doesn't find a package (a directory with an __init__.py file), and then use that as the base dir for imports, as documented here. This is nice for dev testing since it means devs don't have to install each time they change code, but causes problems here since the source tree no longer contains builds of python/C extensions (because now they're installed from conda). There is an option to tell pytest to add the source tree to the end of the import path - which should resolve this - but other teams like cuML (cc @dantegd ) preferred to just remove the test __init__.py file. Since it's good to be consistent with other teams, and because our tests at the moment may not run outside of a dev environment (they have additional deps not listed in our conda recipe) and therefore shouldn't be an importable package, removing cugraph.tests as a package makes sense. We may revisit this later if we realize there's advantages to keeping cugraph.tests as an importable package.
  • The cugraph.testing package was added to make it easier to do absolute imports from tests of common utilities, since cugraph.tests is no longer a package. This is also something done by other teams such as cuML and cuDF.
  • The sys.path.insert(...) calls are still a problem, however, since these are only used by MG tests, and MG tests are not run as part of CI, these won't break PRs. @Ethyling mentioned he could add a FIXME to these tests since we agreed it would be better if they could be refactored to not have tests importing other tests anyway (instead, have the shared parts refactored into a common file, possibly in cugraph.testing).

Signed-off-by: Jordan Jacobelli <jjacobelli@nvidia.com>
@jjacobelli
Copy link
Contributor Author

@gpucibot merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants