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: change COLL_SCORE separator #142

Merged
merged 1 commit into from
Apr 8, 2021

Conversation

vspetrov
Copy link
Collaborator

@vspetrov vspetrov commented Apr 5, 2021

Changes separator character in UCC_*_COLL_SCORE parsing from ";"
to "#". The reason is that some runtimes (MPIs, e.g. OpenMPI)
incorrectly parse ";" token. W/o that change one has to always use
quote marks to guard the string.

This is just minor usability improvement.

@manjugv manjugv added this to the v0.1.0 Release milestone Apr 5, 2021
@manjugv manjugv added the Target: v0.1.x PRs/Issue for the v0.1.x release label Apr 5, 2021
@vspetrov vspetrov force-pushed the topic/score_separator branch from 20e565a to b8fa565 Compare April 6, 2021 05:36
@vspetrov
Copy link
Collaborator Author

vspetrov commented Apr 6, 2021

After short discussion with Sergey we think we want more input from UCC people. Should we keep it ";" and then user will need to guard the string with "" in some runtimes (e.g. OMPI). Or should we change , like in this PR. This is a very minor readability question. My preference to change to # and don't think about quotes. what do other people think?

@bureddy
Copy link
Collaborator

bureddy commented Apr 6, 2021

will "," work?

@vspetrov
Copy link
Collaborator Author

vspetrov commented Apr 7, 2021

will "," work?

no, "," is already in use. Example cmd line: UCC_TL_UCP_COLL_SCORE=allreduce,barrier:cuda,host:0-1M:inf;bcast,alltoall:0
comma is used a separator in multiple qualifiers

@vspetrov
Copy link
Collaborator Author

vspetrov commented Apr 7, 2021

btw, since we are discussing this now: currently the parameter is called UCC_CL/TL_*_COLL_SCORE. Is "COLL" redundant" here? maybe just UCC_TL_UCP_SCORE?

@manjugv
Copy link
Contributor

manjugv commented Apr 7, 2021

btw, since we are discussing this now: currently the parameter is called UCC_CL/TL_*_COLL_SCORE. Is "COLL" redundant" here? maybe just UCC_TL_UCP_SCORE?

I think UCC_TL_UCP_SCORE makes sense.

    Changes scoring env var name: drop "COLL". Now the params are
    called UCC_TL/CL_<name>_SCORE.

    Changes separator charactor in UCC_*_SCORE parsing from ";"
    to "#". The reason is that some runtimes (MPIs, e.g. OpenMPI)
    incorrectly parse ";" token. W/o that change one has to always use
    quote marks to guard the string.

    This is just minor usability improvement.
@vspetrov vspetrov force-pushed the topic/score_separator branch from b8fa565 to f9c97c9 Compare April 8, 2021 08:32
@vspetrov vspetrov merged commit a094452 into openucx:master Apr 8, 2021
@vspetrov vspetrov deleted the topic/score_separator branch April 8, 2021 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready-for-Review Target: v0.1.x PRs/Issue for the v0.1.x release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants