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

Validate environment variables prefixed with KOKKOS_ #6847

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

brian-kelley
Copy link
Contributor

@brian-kelley brian-kelley commented Feb 29, 2024

For any environment variables prefixed with KOKKOS_ (case insensitive), check that it is one of the recognized variables that kokkos or kokkos-tools uses. Give a warning if it's unrecognized, or if it's recognized but not in the correct case (all-caps).

This is based off work @cwpearson did for Tpetra in trilinos/Trilinos#12722.
The checking is done once during initialize_internal().

Example usage:

export KOKKOS_NOT_A_VALID_VAR=123
export KOKKOS_device_id=0
./example 10

prints:

Warning: environment variable "KOKKOS_device_id" is
not in all-caps, but is otherwise one of the valid
environment variables used by Kokkos. If you intended
to set "KOKKOS_DEVICE_ID", this variable
will not have the same effect.

Warning: environment variable "KOKKOS_NOT_A_VALID_VAR"
is prefixed like a variable that controls Kokkos,
but is not in the list of valid
Kokkos environment variables.
...

For any environment variables prefixed with "KOKKOS_" (case
insensitive), check that it is one of the recognized variables
that kokkos or kokkos-tools uses. Give a warning if it's unrecognized,
or if it's recognized but not in the correct case (all-caps).
@brian-kelley brian-kelley added the Enhancement Improve existing capability; will potentially require voting label Feb 29, 2024
@brian-kelley brian-kelley changed the title Validate environment variables "KOKKOS_..." Validate environment variables prefixed with KOKKOS_ Feb 29, 2024
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

  • We need some way of programmatically of disabling warnings for environment variables that match a user-specified regex like we do for command line arguments
    namespace {
    std::vector<std::regex> do_not_warn_regular_expressions{
    std::regex{"--kokkos-tool.*", std::regex::egrep},
    };
    }
    void Kokkos::Impl::do_not_warn_not_recognized_command_line_argument(
    std::regex ignore) {
    do_not_warn_regular_expressions.push_back(std::move(ignore));
    }
  • You need to add tests. See examples here
    TEST(defaultdevicetype, env_vars_device_id) {
    EnvVarsHelper ev = {{
    {"KOKKOS_DEVICE_ID", "33"},
    }};
    SKIP_IF_ENVIRONMENT_VARIABLE_ALREADY_SET(ev);
    Kokkos::InitializationSettings settings;
    Kokkos::Impl::parse_environment_variables(settings);
    EXPECT_TRUE(settings.has_device_id());
    EXPECT_EQ(settings.get_device_id(), 33);
    }
  • We need to make sure we don't raise warnings for env variables that are commonly set by modules on leadership computing facilities. I expect things like Kokkos_ROOT, Kokkos_DIR, Kokkos_HOME, etc.

@brian-kelley brian-kelley requested a review from dalg24 March 1, 2024 00:46
@brian-kelley
Copy link
Contributor Author

@dalg24 I think I addressed your points. Can you think of other variables that should be ignored (from modules etc.)?

@@ -24,9 +24,16 @@
#include <cstring>
#include <iostream>
#include <regex>
#include <unordered_set>
Copy link
Member

Choose a reason for hiding this comment

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

Forgot to remove

std::regex{"KOKKOS_SRC=.*",
std::regex::egrep | std::regex_constants::icase},
std::regex{"KOKKOS_INSTALL=.*",
std::regex::egrep | std::regex_constants::icase},
Copy link
Member

Choose a reason for hiding this comment

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

Let's discuss that list at the next developer meeting.
We need to check with other projects in the Kokkos ecosystem that this works for them.

I purposefully omitted KOKKOS_VARIORUM_FUNC_TYPE, I don't think we should accommodate this one.

Kokkos::Impl::parse_environment_variables(settings);
auto captured = ::testing::internal::GetCapturedStderr();
EXPECT_TRUE(captured.find("KOKKOS_EXTENSION_WHATEVER") ==
std::string::npos);
Copy link
Member

Choose a reason for hiding this comment

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

This is kinda awkward. We cannot actually assert that there will be no warnings at all. I am wondering whether we should add a leading char* envp[] argument to Impl::parse_environment_variables() so we can control more fine-grained. What do you think?

auto captured = ::testing::internal::GetCapturedStderr();
EXPECT_TRUE(captured.find("not recognized") != std::string::npos &&
captured.find("KOKKOS_device_id=0") != std::string::npos &&
!settings.has_device_id());
Copy link
Member

Choose a reason for hiding this comment

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

The windows build failed with

[ RUN      ] defaultdevicetype.env_vars_unrecognized
C:\projects\source\core\unit_test\TestParseCmdLineArgsAndEnvVars.cpp(418): error: Value of: captured.find("not recognized") != std::string::npos && captured.find("KOKKOS_device_id=0") != std::string::npos && !settings.has_device_id()
  Actual: false
Expected: true
[  FAILED  ] defaultdevicetype.env_vars_unrecognized (18 ms)

@nliber
Copy link
Contributor

nliber commented Mar 6, 2024

The only variable exported form kokkos modules at ALCF is KOKKOS_HOME

@dalg24
Copy link
Member

dalg24 commented Mar 6, 2024

The only variable exported form kokkos modules at ALCF is KOKKOS_HOME

OLCF prefixed them with OLCF_ so no issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improve existing capability; will potentially require voting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants