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

CORE: Use uint64_t for bitmap configs #1047

Merged
merged 1 commit into from
Dec 20, 2024
Merged

Conversation

andylin-hao
Copy link
Contributor

What

Make the bitmap-type config profile_mode uint64_t to align with recent UCX(S) changes.

Why ?

UCX(S) now uses uint64_t for bitmap-type (declared via UCS_CONFIG_TYPE_BITMAP) configurations (see openucx/ucx@8a84af8). UCC should align with this change, otherwise setting bitmap-type config may overwrite the next 4-byte memory during config parsing (see openucx/ucx@8a84af8#diff-8dd28fc04f39623f32e719552443482db8a07986754b3ba0ba48387db94d8bbdL446). In the case of UCC, the profile_file option of ucc_global_opts will be overwritten when profile_mode is set.

@swx-jenkins3
Copy link

Can one of the admins verify this patch?

Copy link
Contributor

@Sergei-Lebedev Sergei-Lebedev left a comment

Choose a reason for hiding this comment

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

nice catch, thanks

@Sergei-Lebedev
Copy link
Contributor

ok to test

@manjugv
Copy link
Contributor

manjugv commented Nov 12, 2024

@andylin-hao Thanks. Which org do you represent?

@andylin-hao
Copy link
Contributor Author

andylin-hao commented Nov 13, 2024

@manjugv Hi, I'm currently a PhD student (https://github.com/andylin-hao) researching on GPU communications. Just came across this problem when researching UCC/X out of curiosity. So I'm not really representing any specfic orgs in this case, though my official affiliation is with Tsinghua University.

@manjugv
Copy link
Contributor

manjugv commented Nov 14, 2024

@andylin-hao Will you be able to sign a CLA? [1] Without that, we cannot accept the change.

[1] https://github.com/openucx/ucc?tab=readme-ov-file#contributing

@andylin-hao
Copy link
Contributor Author

@manjugv Sure, I just did. Could you let me know to whom should I send the signed CLA?

@manjugv
Copy link
Contributor

manjugv commented Nov 15, 2024

@andylin-hao DM'ed you details.

@janjust
Copy link
Collaborator

janjust commented Dec 9, 2024

@manjugv @andylin-hao was the CLA signed - can we merge this?

@andylin-hao
Copy link
Contributor Author

@andylin-hao was the CLA signed?

Yeah, I signed it.

@manjugv
Copy link
Contributor

manjugv commented Dec 19, 2024

@janjust It was signed.

@Sergei-Lebedev Sergei-Lebedev enabled auto-merge (squash) December 20, 2024 10:01
@Sergei-Lebedev Sergei-Lebedev merged commit 0c0fc21 into openucx:master Dec 20, 2024
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants