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.

Sign up for GitHub

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

Jump to bottom

Introduce DALI_PRELOAD_PLUGINS #5457

Merged
merged 8 commits into from
May 9, 2024
Merged

Introduce DALI_PRELOAD_PLUGINS #5457

merged 8 commits into from
May 9, 2024

Conversation

jantonguirao
Copy link
Contributor

@jantonguirao jantonguirao commented May 7, 2024 •
edited
Loading

Category:

New feature

Description:

Introduces a new environment variable that allows the user to preload some DALI plugins:

  • unset: load all plugins under DALI installation directory in a subfolder called plugins
  • set: contains a list of paths separated by colon. Each path can be a plugin or a directory, in which case will be scanned for plugins. A DALI plugin follows the filename libdali_{plugin_name}.so

Additional information:

Affected modules and functionalities:

Plugin manager

Key points relevant for the review:

Tests:

  • Existing tests apply
  • New tests added
    • Python tests
    • GTests
    • Benchmark
    • Other
  • N/A

Checklist

Documentation

  • Existing documentation applies
  • Documentation updated
    • Docstring
    • Doxygen
    • RST
    • Jupyter
    • Other
  • N/A

DALI team only

Requirements

  • Implements new requirements
  • Affects existing requirements
  • N/A

REQ IDs: N/A

JIRA TASK: N/A

@jantonguirao
Copy link
Contributor Author

!build

@jantonguirao jantonguirao force-pushed the autodiscover_plugins branch from c9ae231 to 6aa365b Compare May 7, 2024 14:24
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [14816252]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [14816252]: BUILD FAILED

@jantonguirao
Introduce DALI_PRELOAD_PLUGINS
7f76bd9 
Signed-off-by: Joaquin Anton <janton@nvidia.com>
@jantonguirao jantonguirao force-pushed the autodiscover_plugins branch from 6aa365b to 6385970 Compare May 7, 2024 15:00
@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [14816954]: BUILD STARTED

@jantonguirao
Update documentation
353f81a 
Signed-off-by: Joaquin Anton <janton@nvidia.com>
@jantonguirao jantonguirao force-pushed the autodiscover_plugins branch from 6385970 to 353f81a Compare May 7, 2024 15:02
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [14816986]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [14816986]: BUILD FAILED

@jantonguirao
Fix
a512ad6 
Signed-off-by: Joaquin Anton <janton@nvidia.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [14817292]: BUILD STARTED

@awolant awolant self-assigned this May 7, 2024
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [14817292]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [14838996]: BUILD STARTED

@jantonguirao
Fix TL0_abi
6760609 
Signed-off-by: Joaquin Anton <janton@nvidia.com>
@jantonguirao jantonguirao force-pushed the autodiscover_plugins branch from f650505 to 6760609 Compare May 8, 2024 07:57
jantonguirao
jantonguirao commented May 8, 2024
View reviewed changes
dali/test/dali_plugin_manager_test.cc

void TestPlugin(const std::string &backend) {
LoadDummyPlugin();
void TestPlugin(const std::string& backend) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the test fixture because it was completely unnecessary

jantonguirao
jantonguirao commented May 8, 2024
View reviewed changes
dali/test/dali_plugin_manager_test.cc
TEST(DummyTest, TestPluginCPU) {
GTEST_FLAG_SET(death_test_style, "threadsafe");
LoadDummyPlugin();
EXPECT_EXIT(TestPlugin("cpu"), testing::ExitedWithCode(0), "");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is crucial. Before this change it was enough that the first testcase loads the plugin, as it was run in the same process. This change makes sure that each test case has to load the plugin

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be good to add a comment here to explain this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [14838996]: BUILD FAILED

@jantonguirao
Add comment
6e2f2cf 
Signed-off-by: Joaquin Anton <janton@nvidia.com>
klecki
klecki reviewed May 8, 2024
View reviewed changes
docs/examples/custom_operations/custom_operator/create_a_custom_operator.ipynb
@@ -43,47 +43,47 @@
"name": "stdout",
Copy link
Contributor

@klecki klecki May 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Should we get libdali_cusdomdummy.so here?


Reply via ReviewNB

@jantonguirao
Fixes
e78779c 
Signed-off-by: Joaquin Anton <janton@nvidia.com>
@jantonguirao
Copy link
Contributor Author

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [14875217]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [14875217]: BUILD FAILED

klecki
klecki approved these changes May 9, 2024
View reviewed changes
Copy link
Contributor

@klecki klecki left a comment

Choose a reason for hiding this comment

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

Minor nitpicks, otherwise ok.

docs/examples/custom_operations/custom_operator/create_a_custom_operator.ipynb Outdated
"[ 66%] \u001b[32mBuilding CUDA object CMakeFiles/customdummy.dir/dummy.cu.o\u001b[0m\n",
"[100%] \u001b[32m\u001b[1mLinking CXX shared library libcustomdummy.so\u001b[0m\n",
"[100%] Built target customdummy\n"
"-- Build files have been written to: /home/janton/git/dali/docs/examples/custom_operations/custom_operator/customdummy/build\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove user-specific paths.

dali/plugin/plugin_manager.cc Outdated
Comment on lines 75 to 90
while (index != string::npos) {
auto plugin_path = dali_preload_plugins.substr(previous, index - previous);
if (fs::is_directory(plugin_path)) {
PluginManager::LoadDirectory(plugin_path);
} else {
PluginManager::LoadLibrary(plugin_path);
}
previous = index + 1;
index = dali_preload_plugins.find(delimiter, previous);
}
auto plugin_path = dali_preload_plugins.substr(previous);
if (fs::is_directory(plugin_path)) {
PluginManager::LoadDirectory(plugin_path);
} else {
PluginManager::LoadLibrary(plugin_path);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: You can probably handle this with a do/while

dali/plugin/plugin_manager.cc Outdated
size_t previous = 0;
std::string preload(dali_preload_plugins);
size_t index = dali_preload_plugins.find(delimiter);
std::vector<std::string> plugins;
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems unused, you just push to it once at the end.

@jantonguirao
Remove user specific paths
45c9475 
Signed-off-by: Joaquin Anton <janton@nvidia.com>
@jantonguirao jantonguirao force-pushed the autodiscover_plugins branch from 5277cec to 6913856 Compare May 9, 2024 11:51
@jantonguirao
Code review fixes
495b4b0 
Signed-off-by: Joaquin Anton <janton@nvidia.com>
@jantonguirao jantonguirao force-pushed the autodiscover_plugins branch from 6913856 to 495b4b0 Compare May 9, 2024 12:35
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [14880642]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [14880642]: BUILD FAILED

@jantonguirao jantonguirao merged commit dfc7d95 into NVIDIA:main May 9, 2024
5 of 6 checks passed
stiepan added a commit to stiepan/DALI that referenced this pull request May 14, 2024
@stiepan
Revert "Introduce DALI_PRELOAD_PLUGINS (NVIDIA#5457)"
77dc203 
This reverts commit dfc7d95.

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awolant awolant awolant approved these changes

@klecki klecki klecki approved these changes

Assignees

@klecki klecki

@awolant awolant

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jantonguirao @dali-automaton @klecki @awolant