Skip to content

UCS/UCT/IB: Select SL not depending on AR support by default - #5943

Merged
yosefe merged 1 commit into
openucx:masterfrom
dmitrygx:topic/uct/ib_sl_auto
Nov 26, 2020
Merged

yosefe merged 1 commit into
openucx:masterfrom
dmitrygx:topic/uct/ib_sl_auto

Conversation

@dmitrygx

Copy link
Copy Markdown
Member

What

Select SL not depending on AR support by default.

Why ?

To align with previous behavior, since users may not expect that UCX selects SL with AR (if possible) or SL without AR (if possbile) after #5869.

How ?

  1. Introduced new value for ucs_ternary_value_t - "auto" that select default value for ternary value.
  2. Handled "auto" value when selecting SL. If "auto" is set, UCT/IB/MLX5 selects SL requested by a user (thru UCX_IB_SL=<sl>) or 0.
  3. Updated UCT/IB gtests accordingly and fix bug when verifying OK case (some case wasn't handled, e.g. UCS_NO).

@dmitrygx

Copy link
Copy Markdown
Member Author

@yosefe @brminich wdyt is the best option from the items below?

  • having ucs_ternary_value_t with UCS_AUTO added
  • rename ucs_ternary_value_t to ucs_ynta_velue_t (i.e. YNTA = { Yes, No, Try, Auto })

Comment thread src/uct/ib/mlx5/ib_mlx5.c Resolved
Comment thread src/uct/ib/mlx5/ib_mlx5.c Resolved
@brminich

Copy link
Copy Markdown
Contributor

@yosefe @brminich wdyt is the best option from the items below?

  • having ucs_ternary_value_t with UCS_AUTO added
  • rename ucs_ternary_value_t to ucs_ynta_velue_t (i.e. YNTA = { Yes, No, Try, Auto })

Maybe add ucs_ynta_velue_t as a new type?

@yosefe

yosefe commented Nov 24, 2020

Copy link
Copy Markdown
Member

Maybe add ucs_ynta_velue_t as a new type?

+1

@dmitrygx

Copy link
Copy Markdown
Member Author

Maybe add ucs_ynta_velue_t as a new type?

+1

but, it will have the same UCS_YES/UCS_NO/UCS_TRY that are the same as for ucs_ternary_value_t

@brminich

brminich commented Nov 24, 2020 •

Copy link
Copy Markdown
Contributor

but, it will have the same UCS_YES/UCS_NO/UCS_TRY that are the same as for ucs_ternary_value_t

Yes, but you will not need to define/implement handling of "auto" for all existing ternary vars

@dmitrygx

Copy link
Copy Markdown
Member Author

but, it will have the same UCS_YES/UCS_NO/UCS_TRY that are the same as for ucs_ternary_value_t

Yes, but you will not need to define/implement handling of "auto" for all existing ternary vars

they should have different names then

@yosefe

yosefe commented Nov 24, 2020

Copy link
Copy Markdown
Member

they should have different names then

UCS_TERNARY_[YES|NO|TRY]
UCS_YNTA_[YES|NO|TRY|AUTO]

@dmitrygx

Copy link
Copy Markdown
Member Author

they should have different names then

UCS_TERNARY_[YES|NO|TRY]
UCS_YNTA_[YES|NO|TRY|AUTO]

then it will be impossible to re-use code for ternary to handle yes/no/try
what about removing ternary type and have ynta?

@yosefe

yosefe commented Nov 24, 2020

Copy link
Copy Markdown
Member

then it will be impossible to re-use code for ternary to handle yes/no/try
what about removing ternary type and have ynta?

What will we do in code which expects only yes/no/try options?

@dmitrygx

Copy link
Copy Markdown
Member Author

then it will be impossible to re-use code for ternary to handle yes/no/try
what about removing ternary type and have ynta?

What will we do in code which expects only yes/no/try options?

I see
@yosefe wdyt about changing ucs_ternary_value_t -> ucs_ternary_auto_value_t? and we will have UCS_CONFIG_TYPE_TERNARY and UCS_CONFIG_TYPE_TERNARY_AUTO like we have for UCS_CONFIG_TYPE_ON_OFF/UCS_CONFIG_TYPE_ON_OFF_AUTO. sscanf() parsing function will be different for them to handle or not handle auto value

@dmitrygx

Copy link
Copy Markdown
Member Author

then it will be impossible to re-use code for ternary to handle yes/no/try
what about removing ternary type and have ynta?

What will we do in code which expects only yes/no/try options?

I see
@yosefe wdyt about changing ucs_ternary_value_t -> ucs_ternary_auto_value_t? and we will have UCS_CONFIG_TYPE_TERNARY and UCS_CONFIG_TYPE_TERNARY_AUTO like we have for UCS_CONFIG_TYPE_ON_OFF/UCS_CONFIG_TYPE_ON_OFF_AUTO. sscanf() parsing function will be different for them to handle or not handle auto value

@yosefe implemented it to get your review

@dmitrygx
dmitrygx force-pushed the topic/uct/ib_sl_auto branch from f3e0e0a to e3631d0 Compare November 24, 2020 15:11
Comment thread src/ucs/config/parser.c Resolved
Comment thread src/ucs/config/parser.c Resolved
Comment thread src/ucs/config/parser.c Resolved
Comment thread test/gtest/uct/ib/test_ib.cc Resolved
@dmitrygx
dmitrygx force-pushed the topic/uct/ib_sl_auto branch from a62127e to 583b37f Compare November 25, 2020 11:55
@dmitrygx

Copy link
Copy Markdown
Member Author

bot:pipe:retest

@yosefe

yosefe commented Nov 25, 2020

Copy link
Copy Markdown
Member

/azp run

@yosefe

yosefe commented Nov 25, 2020

Copy link
Copy Markdown
Member

bot:pipe:retest

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@dmitrygx

Copy link
Copy Markdown
Member Author

bot:pipe:retest

@dmitrygx

Copy link
Copy Markdown
Member Author

@yosefe can we merge ignoring io demo failure?
it is unrelated to the PR

@yosefe
yosefe merged commit aa94804 into openucx:master Nov 26, 2020
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.

3 participants