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

Update packages (pybind11 and catch2) and do not use nvidia-docker command #618

Merged
merged 5 commits into from
Oct 30, 2023

Conversation

gigony
Copy link
Contributor

@gigony gigony commented Oct 25, 2023

Update Catch2 to v3.4.0

Without upgrading Catch2, The following error occurs when building on
Ubuntu 22.04 due to glibc:

cucim/build-debug/_deps/deps-catch2-src/single_include/catch2/catch.hpp:10830:58: error: call to non-‘constexpr’ function
‘long int sysconf(int)’
10830 |     static constexpr std::size_t sigStackSize = 32768 >=  MINSIGSTKSZ ? 32768 : MINSIGSTKSZ;

Update pybind11 to v2.11.1

Even with the latest version of pybind11, we still have an issue
with pybind11::array_t when cuCIM is used in multithread without
importing numpy in the main thread.

See pybind/pybind11#4877

Will need to wait for the next release of pybind11.

Use runtime option instead of using nvidia-docker command

nvidia-docker binary is not available if user doesn't install
nvidia-docker2 package. This change uses runtime option instead
of using nvidia-docker command.

Apply pybind11 patch to avoid deadlock (until new release is available)

This applies the following patches to pybind11:

to avoid deadlock when using pybind11 without importing numpy in
multi-threaded environment.

@gigony gigony added non-breaking Introduces a non-breaking change maintenance labels Oct 25, 2023
@gigony gigony self-assigned this Oct 25, 2023
@gigony gigony requested review from a team as code owners October 25, 2023 01:23
@copy-pr-bot
Copy link

copy-pr-bot bot commented Oct 25, 2023

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@grlee77 grlee77 added the improvement Improves an existing functionality label Oct 25, 2023
@grlee77
Copy link
Contributor

grlee77 commented Oct 25, 2023

/ok to test

@grlee77
Copy link
Contributor

grlee77 commented Oct 25, 2023

(@gigony, I sent you a slack message with info on how to comply with newer RAPIDS signing requirements, so adding "ok to test" comments won't be required for CI to run)

Copy link
Contributor

@grlee77 grlee77 left a comment

Choose a reason for hiding this comment

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

Thanks, @gigony. It looks good to me.

Without upgrading Catch2, The following error occurs when building on
Ubuntu 22.04 due to glibc:

cucim/build-debug/_deps/deps-catch2-src/single_include/catch2/
catch.hpp:10830:58: error: call to non-‘constexpr’ function
‘long int sysconf(int)’
10830 |     static constexpr std::size_t sigStackSize = 32768 >=
                MINSIGSTKSZ ? 32768 : MINSIGSTKSZ;

Signed-off-by: Gigon Bae <[email protected]>
This PR updates pybind11 to v2.11.1.

Even with the latest version of pybind11, we still have an issue
with `pybind11::array_t` when cuCIM is used in multithread without
importing numpy in the main thread.

pybind/pybind11#4877

Will need to wait for the next release of pybind11.

Signed-off-by: Gigon Bae <[email protected]>
nvidia-docker binary is not available if user doesn't install
nvidia-docker2 package. This change uses runtime option instead
of using nvidia-docker command.

Signed-off-by: Gigon Bae <[email protected]>
This applies the following patches to pybind11:

- pybind/pybind11#4857
- pybind/pybind11#4877

to avoid deadlock when using pybind11 without importing numpy in
multi-threaded environment.
@jakirkham jakirkham added this to the v23.12.00 milestone Oct 27, 2023
@jakirkham
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit cd54957 into rapidsai:branch-23.12 Oct 30, 2023
26 checks passed
@jakirkham
Copy link
Member

Thanks all! 🙏

rapids-bot bot pushed a commit that referenced this pull request Oct 30, 2023
MERGE after #618

### Fix memory corruption issue in ThreadBatchDataLoader

Fix #592    

- Return a shared pointer of CuImageIterator, taking ownership of CuImageIterator instance, instead of returning a copy of CuImageIterator
- Do not create new instance for CuImageIterator when `__iter__` is called. Instead, return self.
- Wait until all threads are finished in ThreadBatchDataLoader destructor

### Fix memory leak in ThreadBatchDataLoader

Fix #598
    
- Throw StopIteration exception early
- Do not set nullptr for `raster_data_` in `ThreadBatchDataLoader::next_data()`
  - Setting nullptr prevents freeing memory in the destructor of ThreadBatchDataLoader

### Fix fatal errors for cache object

- GIL is not acquired when using Python object.

### Add iterator-related test cases for memory issue

Authors:
  - Gigon Bae (https://github.com/gigony)

Approvers:
  - Gregory Lee (https://github.com/grlee77)
  - https://github.com/jakirkham

URL: #620
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality maintenance non-breaking Introduces a non-breaking change
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants