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

Build CPP tests in new CI workflow #6947

Merged
merged 27 commits into from
Apr 26, 2024
Merged

Build CPP tests in new CI workflow #6947

merged 27 commits into from
Apr 26, 2024

Conversation

will-cromar
Copy link
Collaborator

@will-cromar will-cromar commented Apr 19, 2024

  • Pre-build CPP tests during build step and upload them as artifacts. This significantly speeds up getting feedback from these tests.
    • If the build options for the torch_xla package and the C++ tests, this invalidates the cache. Rather than trying to bring test/cpp/run_test.sh in line with setup.py, we can just build everything at once
    • Support building C++ tests in ansible so we use exactly the same build options.
  • Create a new test_suite for CPP tests
  • Add placeholder plugin that just returns a library path and default options. This is for C++ tests only for now.
  • Remove old workflow.

A typical run in the old CI took about 2.5 hours. This test run took only 1 hour and 10 minutes. Most tests in the new workflow finish before the old build finishes, e.g.

image

@will-cromar will-cromar changed the title [WIP] Build CPP tests in new CI workflow Build CPP tests in new CI workflow Apr 23, 2024
@will-cromar will-cromar marked this pull request as ready for review April 23, 2024 18:10
Comment on lines +24 to +38
// Placeholder plugin for testing only. Does not implement multiprocessing or
// configuration. Very likely will not work from Python code.
class LibraryPlugin : public PjRtPlugin {
public:
std::string library_path() const override {
return sys_util::GetEnvString("PJRT_LIBRARY_PATH", "");
}

const std::unordered_map<std::string, xla::PjRtValueType>
client_create_options() const override {
return {};
}

bool requires_xla_coordinator() const override { return false; }
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need this plugin?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just a minimal implementation of the plugin interface that lets us pass in a path. We want to take this codepath during init, rather than the statically linked CUDA client:

if (sys_util::GetEnvBool(env::kEnvPjrtDynamicPlugins, false) &&

Normally the plugin registration is done through Python, so we need a workaround for the C++ tests.

@will-cromar will-cromar requested a review from lsy323 as a code owner April 26, 2024 20:04
@will-cromar will-cromar merged commit b834e49 into master Apr 26, 2024
20 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants