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

Add cucim.kit.cumed plugin with skeleton #129

Merged
merged 4 commits into from
Nov 3, 2021

Conversation

gigony
Copy link
Contributor

@gigony gigony commented Oct 13, 2021

This change adds a skeleton plugin named cucim.kit.cumed for medical-related file loaders and image operations (e.g., .mhd file format support).

The image loader to use is determined by is_valid() method implemented in each plugin and CuImage object holds the reference to the image format descriptor (cucim::io::format::ImageFormatDesc*).

cuCIM treats cucim.kit.cuslide and cucim.kit.cumed plugins as built-in plugins, and it would support custom plugins in the future (by refactoring the framework code, according to the recent update of Carbonite (minimal) SDK library and use the library as it is).

@gigony gigony added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Oct 13, 2021
@gigony gigony added this to the v21.10.01 milestone Oct 13, 2021
@gigony gigony self-assigned this Oct 13, 2021
@gigony gigony requested review from a team as code owners October 13, 2021 05:13
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
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 Gigon! 😄

Had a couple questions below

cpp/include/cucim/core/framework.h Show resolved Hide resolved
ImageFormat() = default;
~ImageFormat() = default;

bool add_interfaces(cucim::io::format::IImageFormat* image_formats);
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to use a reference here? Could the variable be const?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing it out!
Updated to use const variable.


// Dynamically allocate memory for json_data (need to be freed manually);
const std::string& json_str = std::string{};
char* json_data_ptr = static_cast<char*>(cucim_malloc(json_str.size() + 1));
Copy link
Member

Choose a reason for hiding this comment

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

Is there a corresponding free somewhere? Would it be possible to use RAII to handle this allocation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is at the destructor of CuImage class.

cucim/cpp/src/cuimage.cpp

Lines 236 to 241 in 95e4483

// Memory for json_data needs to be manually released if image_metadata_->json_data is not ""
if (image_metadata_->json_data && *image_metadata_->json_data != '\0')
{
cucim_free(image_metadata_->json_data);
image_metadata_->json_data = nullptr;
}

I have created an issue to handle general resource handling issues and will follow up on the improvement through the issue.


TEST_CASE("Verify file", "[test_basic.cpp]")
{
REQUIRE(1 == 1);
Copy link
Member

Choose a reason for hiding this comment

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

Were we wanting to add more here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, it is just a placeholder to implement unit tests for cumed plugin.

@gigony
Copy link
Contributor Author

gigony commented Oct 30, 2021

@ajschmidt8 It seems that it has an issue with cuda11.5 (same for #128)

https://gpuci.gpuopenanalytics.com/job/rapidsai/job/gpuci-cuda115/job/cucim/job/prb/job/cucim-gpu-test/CUDA=11.5,GPU_LABEL=cuda115,LINUX_VER=centos7,PYTHON=3.8/3/console

19:01:04 ++ python -c 'import cucim'
19:01:05 ci/gpu/build.sh: line 91:  1680 Segmentation fault      (core dumped) python -c 'import cucim'
19:01:05 Build step 'Execute shell' marked build as failure
19:01:08 Recording test results

Is there a known issue regarding this?

Update: It seems so. I can see failures in other projects - rapidsai/cudf#9562

@ajschmidt8
Copy link
Member

@ajschmidt8 It seems that it has an issue with cuda11.5 (same for #128)

https://gpuci.gpuopenanalytics.com/job/rapidsai/job/gpuci-cuda115/job/cucim/job/prb/job/cucim-gpu-test/CUDA=11.5,GPU_LABEL=cuda115,LINUX_VER=centos7,PYTHON=3.8/3/console

19:01:04 ++ python -c 'import cucim'
19:01:05 ci/gpu/build.sh: line 91:  1680 Segmentation fault      (core dumped) python -c 'import cucim'
19:01:05 Build step 'Execute shell' marked build as failure
19:01:08 Recording test results

Is there a known issue regarding this?

Update: It seems so. I can see failures in other projects - rapidsai/cudf#9562

Yes, 11.5 tests are optional right now and won't prevent merging. We're still working out some build issues. Any errors unrelated to this repo are probably fine to ignore for now.

@gigony
Copy link
Contributor Author

gigony commented Nov 2, 2021

Thanks @ajschmidt8 for the confirmation!

@jakirkham
Copy link
Member

@gpucibot merge

@rapids-bot rapids-bot bot merged commit e521dd4 into rapidsai:branch-21.12 Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants