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 compilers #10275

Merged
merged 3 commits into from
May 23, 2022
Merged

Use conda compilers #10275

merged 3 commits into from
May 23, 2022

Conversation

jjacobelli
Copy link
Contributor

@jjacobelli jjacobelli commented Feb 12, 2022

This PR enables the usage of conda compilers to build conda packages. This is required to use mambabuild

@jjacobelli jjacobelli added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Feb 12, 2022
@jjacobelli jjacobelli self-assigned this Feb 12, 2022
@github-actions github-actions bot added the conda label Feb 12, 2022
@jjacobelli jjacobelli force-pushed the conda-comp branch 9 times, most recently from 15d504d to a505aae Compare February 14, 2022 14:05
@github-actions github-actions bot added CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels Feb 14, 2022
@github-actions github-actions bot removed CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels Feb 14, 2022
@jjacobelli jjacobelli force-pushed the conda-comp branch 3 times, most recently from ce1fa53 to 84ef8eb Compare February 15, 2022 10:05
@github-actions github-actions bot added CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels Feb 15, 2022
@jjacobelli jjacobelli force-pushed the conda-comp branch 5 times, most recently from cabe930 to b2e7b02 Compare February 15, 2022 20:58
conda/recipes/cudf/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/custreamz/meta.yaml Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the CMake CMake build issue label May 20, 2022
Signed-off-by: Jordan Jacobelli <[email protected]>
@github-actions github-actions bot removed the libcudf Affects libcudf (C++/CUDA) code. label May 20, 2022
Signed-off-by: Jordan Jacobelli <[email protected]>
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Jordan! 😄

Generally this looks great. Had a few suggested simplifications below

conda/recipes/dask-cudf/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/custreamz/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/cudf/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/cudf_kafka/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/libcudf/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/libcudf/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/libcudf/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/libcudf/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/libcudf/meta.yaml Outdated Show resolved Hide resolved
conda/recipes/libcudf/meta.yaml Outdated Show resolved Hide resolved
Signed-off-by: Jordan Jacobelli <[email protected]>
@jjacobelli
Copy link
Contributor Author

@jakirkham PR updated

@jakirkham
Copy link
Member

Thanks Jordan! 🙏 LGTM 😄

@vyasr
Copy link
Contributor

vyasr commented May 21, 2022

It looks like the most recent release of fsspec broke a few of our tests while our CI was offline. The issue is fsspec/filesystem_spec#961, which changes the behavior of fsspec.core.get_fs_token_path to correctly handle all globs. Previously, that function only handled globs containing an asterisk *, but that PR fixed the function to also handle ? and []. The problem is that we have tests of cudf.read_json with strings containing [] (because they contain lists), and when those globs read this line in the code they correspond to no preexisting files and so instead of getting a path to a nonexistent file like we would have before, we instead get an empty list of paths. That in turn leads to us hitting this error case.

Specifically, here is the difference:

# The new behavior
>>> fsspec.get_fs_token_paths(['[true]'], mode='rb')
(<fsspec.implementations.local.LocalFileSystem object at 0x7f93c7d58040>, '00e91f08872e08eaa06e0006dfc00ab6', [])

# The old behavior. Note that the third element of the tuple was not an empty string, but instead a path.
>>> fsspec.get_fs_token_paths(['[]'], mode='rb')
(<fsspec.implementations.local.LocalFileSystem object at 0x7fa94db5feb0>, '839c5f114235189ec7ed1e0a405abf70', ['/home/vyasr/[]'])

@rjzamora how do you recommend that we fix this? The change to fsspec seems correct, so it's not a bug for us to report upstream but rather something that we need to fix in our usage. I think we probably want to change this error check because using the fact that no paths were returned as an error condition presupposes that the input was a a valid path when it could be an arbitrary JSON string, but I don't have all the context here. Perhaps there is a better solution that addresses problems that I'm not seeing.

@galipremsagar
Copy link
Contributor

It looks like the most recent release of fsspec broke a few of our tests while our CI was offline. The issue is fsspec/filesystem_spec#961, which changes the behavior of fsspec.core.get_fs_token_path to correctly handle all globs. Previously, that function only handled globs containing an asterisk *, but that PR fixed the function to also handle ? and []. The problem is that we have tests of cudf.read_json with strings containing [] (because they contain lists), and when those globs read this line in the code they correspond to no preexisting files and so instead of getting a path to a nonexistent file like we would have before, we instead get an empty list of paths. That in turn leads to us hitting this error case.

Specifically, here is the difference:


# The new behavior

>>> fsspec.get_fs_token_paths(['[true]'], mode='rb')

(<fsspec.implementations.local.LocalFileSystem object at 0x7f93c7d58040>, '00e91f08872e08eaa06e0006dfc00ab6', [])



# The old behavior. Note that the third element of the tuple was not an empty string, but instead a path.

>>> fsspec.get_fs_token_paths(['[]'], mode='rb')

(<fsspec.implementations.local.LocalFileSystem object at 0x7fa94db5feb0>, '839c5f114235189ec7ed1e0a405abf70', ['/home/vyasr/[]'])



@rjzamora how do you recommend that we fix this? The change to fsspec seems correct, so it's not a bug for us to report upstream but rather something that we need to fix in our usage. I think we probably want to change this error check because using the fact that no paths were returned as an error condition presupposes that the input was a a valid path when it could be an arbitrary JSON string, but I don't have all the context here. Perhaps there is a better solution that addresses problems that I'm not seeing.

Yea, I ran into this yesterday. I've worked on a fix. Will open the PR over the weekend.

@vyasr
Copy link
Contributor

vyasr commented May 21, 2022

Thanks @galipremsagar! No rush, I doubt this will seriously block anyone before Monday. I just wanted to diagnose this ASAP.

@galipremsagar
Copy link
Contributor

Opened a PR to address the json pytest failures here: #10924

@Ethyling could you admin merge this(10275) PR since everyone has approved it?

@jjacobelli jjacobelli merged commit f10f380 into rapidsai:branch-22.06 May 23, 2022
rapids-bot bot pushed a commit that referenced this pull request May 23, 2022
Fixes issue described here: #10275 (comment)

This PR removes a false error that says path couldn't be resolved. But that isn't true incase of a json reader where the input can be a json string itself, hence to resolve this issue `is_raw_text_like_input` that indicates an IO reader(like read_json) is calling the utility function and the path need not be a valid one. In case of a true invalid path, either fsspec or libcudf throws a file not found error.

Authors:
  - GALI PREM SAGAR (https://github.com/galipremsagar)

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)
  - Bradley Dice (https://github.com/bdice)

URL: #10924
rapids-bot bot pushed a commit that referenced this pull request May 23, 2022
Dependent on: #10275

This PR adds required `conda` compilers in the environment file

Authors:
  - GALI PREM SAGAR (https://github.com/galipremsagar)

Approvers:
  - AJ Schmidt (https://github.com/ajschmidt8)
  - https://github.com/jakirkham

URL: #10915
rapids-bot bot pushed a commit that referenced this pull request May 24, 2022
As part of conda compiler migration #10275 we switched to `9.x`, this PR removed `9.3` requirement and switches it `9.x`. This is because the following error is seen some machines:
```
Encountered problems while solving.
Problem: package gcc_linux-64-9.3.0-h44160b2_26 requires gcc_impl_linux-64 9.3.0.*, but none of the providers can be installed
```

Upgrading to `9.4.0` on those machines works fine. CI too picks up `9.4.0`, for ex: https://gpuci.gpuopenanalytics.com/job/rapidsai/job/gpuci/job/cudf/job/prb/job/cudf-cpu-cuda-build-arm64/CUDA=11.5/4654/consoleText, hence fixes the broken dev environments.

Authors:
  - GALI PREM SAGAR (https://github.com/galipremsagar)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Ashwin Srinath (https://github.com/shwina)
  - Jordan Jacobelli (https://github.com/Ethyling)

URL: #10943
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.

7 participants