Added GCC-11 and Power support #19
Conversation
|
/ok-to-test |
tarilabs
left a comment
tarilabs
left a comment
There was a problem hiding this comment.
As mentioned offline, in MR we're not using PgSQL mainly due to this limitation:
google#194
We provided a (potential) solution upstream with
google#195
but was never curated for more than a year now.
More in general, we're moving away from MLMD with:
kubeflow/model-registry#865
So in conclusion in MR team we're not using PgSQL<->MLMD and we're moving away from MLMD going fwd.
This PR looks to me mainly focus on PgSQL and other dependency upgrades which in general
/lgtm
also based on the test results documentation provided offline.
I can't advise on the CI failures and which need actual resolution.
Hope this helps!
|
Hi @tarilabs We checked the CI failure and the reason behind looks like it failed to download zetasql repo. When we checked on cloned repo from opendatahub-io/ml-metadata and triggered CI build by pushing to a new branch, there the build gone ahead and it got built successfully. Can you please re-trigger once this build? |
|
retriggered as reqeusted |
|
@tarilabs Hence, we are checking now what all patches are necessary for new commit Id "f764f4e986ac1516ab5ae95e6d6ce2f4416cc6ff" which is getting used. Will push changes with the fix once done. |
PankhudiJ17
force-pushed
the
ppc_support_mlmd
branch
from
5d0c091 to
08aa81c
Compare
|
@tarilabs Can you please trigger build now? |
|
retriggered as requested. |
|
@tarilabs |
|
I'd appreciate an additional reviewer, I've given my LGTM already with #19 (review) but according to process someone else should Approve it. I will eventually be happy to merge it for you. |
|
@tarilabs |
|
@opendatahub-io/data-science-pipelines-maintainers please approve.
|
|
Can someone help to approve and merge the PR? |
I don't think anyone on the team has C++ knowledge that could provide a good review. We don't have any objections with the objective of the PR. |
|
@tsambari2 reach out internally to ODH admins and ask one of them to merge this PR, since no-one involved has C++ expertise to vouch for this Approval+Merge. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe updates introduce a patch for the PostgreSQL dependency to improve compatibility with PowerPC architectures, update the BoringSSL dependency to a new version, and suppress specific compiler warnings in the Red Hat Docker build. Additionally, the Abseil flags system is initialized in the Python metadata store module for testing purposes. Changes
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
WORKSPACE(2 hunks)ml_metadata/metadata_store/metadata_store.py(1 hunks)ml_metadata/third_party/postgresql.patch(1 hunks)ml_metadata/tools/docker_server/Dockerfile.redhat(1 hunks)
🔇 Additional comments (4)
ml_metadata/tools/docker_server/Dockerfile.redhat (1)
40-40: Compiler warning suppression added for GCC-11 compatibility.The addition of
--copt="-Wno-maybe-uninitialized"suppresses warnings about potentially uninitialized variables, which aligns with the PR's goal of supporting GCC-11 builds.While suppressing warnings is generally not ideal, this appears to be a targeted fix for GCC-11 compatibility issues. The change is well-isolated to the build process and doesn't affect runtime behavior.
Please verify that this flag is specifically needed for GCC-11 compatibility and consider adding a comment explaining why this suppression is necessary:
# "-std=c++17" is needed in order to build with ZetaSQL. + # "-Wno-maybe-uninitialized" is needed for GCC-11 compatibility on Power architecture. RUN bazel build -c opt --action_env=PATH \ --define=grpc_no_ares=true \ --copt="-Wno-maybe-uninitialized" \ml_metadata/third_party/postgresql.patch (1)
1-38: Well-structured patch for PowerPC compatibility.The patch correctly addresses PowerPC architecture compatibility issues by:
- Proper architecture detection: Uses comprehensive PowerPC macros (
__ppc__,__PPC__,__ppc64__,__PPC64__)- Safe fallback: Returns
falseforpg_popcount_available()on PowerPC, preventing use of unsupported CPU instructions- Targeted approach: Only wraps the problematic CPU feature detection code without affecting other functionality
- Standard patch format: Follows proper diff conventions and is easy to review
The patch effectively prevents compilation and runtime issues on PowerPC systems by excluding unsupported CPUID instructions and headers. This is a clean, targeted solution that maintains compatibility across different architectures.
WORKSPACE (2)
11-11: PostgreSQL patch integration looks correct.The patch reference correctly points to the PostgreSQL patch file reviewed earlier, which addresses PowerPC architecture compatibility issues.
The integration properly follows Bazel's dependency management conventions and aligns with the PR's goal of supporting Power architecture builds.
56-57: BoringSSL SHA256 checksum verifiedI’ve confirmed that the SHA256 for commit
648cbaf033401b7fe7acdce02f275b06a88aab5cmatches the value in WORKSPACE. This update—intended to improve GCC-11 and Power architecture compatibility—is good to merge.• File: WORKSPACE (lines 56–57, 59)
• Verified SHA256: 579cb415458e9f3642da0a39a72f79fdfe6dc9c1713b3a823f1e276681b9703e
| from absl import flags | ||
| flags.FLAGS(['pytest']) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving flags initialization to a more appropriate location.
Initializing absl flags at module import time with a hardcoded argument can cause issues:
- Side effects during import: Module-level initialization can interfere with other parts of the application that might need different flag configurations
- Hardcoded argument: The
'pytest'argument may not be appropriate for all use cases of this module - Potential conflicts: This could conflict with other flag initialization elsewhere in the codebase
Consider these alternatives:
- Move the flags initialization to a dedicated initialization function
- Use a conditional check to only initialize when running tests
- Initialize flags in the test setup rather than in the main module
-from absl import flags
-flags.FLAGS(['pytest'])
+from absl import flags
+
+def _initialize_flags_for_testing():
+ """Initialize flags for testing purposes."""
+ if not flags.FLAGS.is_parsed():
+ flags.FLAGS(['pytest'])Then call this function in test setup or conditionally based on runtime context.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from absl import flags | |
| flags.FLAGS(['pytest']) | |
| from absl import flags | |
| def _initialize_flags_for_testing(): | |
| """Initialize flags for testing purposes.""" | |
| if not flags.FLAGS.is_parsed(): | |
| flags.FLAGS(['pytest']) |
🤖 Prompt for AI Agents
In ml_metadata/metadata_store/metadata_store.py around lines 20 to 21, the absl
flags are being initialized at module import time with a hardcoded argument
'pytest', which can cause side effects and conflicts. Remove the
flags.FLAGS(['pytest']) call from the module level and instead create a
dedicated initialization function for flags. Call this function conditionally
during test setup or runtime context where needed, avoiding hardcoded arguments
and preventing side effects during normal module imports.
|
proceeding to merge. thanks @grdryn didn't realize you could trigger coderabbit this way 🙏 |
6 participants
This pull request addresses build issues encountered on Power architecture with GCC-11, specifically related to the following components: pg_bitutils.c (PostgreSQL), m4, and BoringSSL. The fixes ensure better compatibility and successful compilation on Power systems using GCC-11.
Summary of Changes
A patch has been added to resolve a build error in pg_bitutils.c encountered specifically on Power architecture.
Patch location: //ml_metadata/third_party:postgresql.patch
Resolved the following compilation error in c-stack.c when building m4 with GCC-11:
error:
missing binary operator before token "(" #elif HAVE_LIBSIGSEGV && SIGSTKSZ < 16384
Upstream patch taken from Debian sources:
https://launchpad.net/ubuntu/+archive/primary/+sourcefiles/m4/1.4.18-5ubuntu1/m4_1.4.18-5ubuntu1.debian.tar.xz
Patch location: ml_metadata/third_party:zetasql.patch
Boringssl Update for GCC-11
A build error was encountered with the previous version of BoringSSL on Power architecture using GCC-11.
The issue was resolved by upgrading the boringssl version.
Update location: WORKSPACE file
Test Case Fix for Abseil (absl)
Patch location: ml_metadata/metadata_store/metadata_store.py
Purpose of this PR:
These changes are necessary to ensure successful builds of ml_metadata on Power architecture with GCC-11. They improve cross-platform compatibility and ensure smoother CI/CD integration for platforms using newer compilers.
Summary by CodeRabbit