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.

Sign up for GitHub

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

Jump to bottom

NEWS: update news for 2.1.2 release #4059

Merged
merged 1 commit into from
Aug 30, 2017
Merged

NEWS: update news for 2.1.2 release #4059

merged 1 commit into from
Aug 30, 2017

Conversation

hppritcha
Copy link
Member

Doing the old-fashioned thing with NEWS.
[skip ci]

Signed-off-by: Howard Pritchard howardp@lanl.gov

@hppritcha hppritcha added this to the v2.1.2 milestone Aug 9, 2017
@hppritcha hppritcha requested a review from jsquyres August 9, 2017 12:58
jsquyres
jsquyres reviewed Aug 9, 2017
View reviewed changes
NEWS Outdated
- Remove IB XRC support from the OpenIB BTL due to lack of support.
- Fix a problem with MPI_IALLTOALLW when using zero-length messages.
Copy link
Member

Choose a reason for hiding this comment

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

For the XRC bullet, it says "support" twice -- perhaps it can be:

Remove IB XRC functionality from the OpenIB BTL due to lack of support.

...the more I think about this, though, are we breaking backwards compatibility? I.e., is the broken XRC functionality a regression since v2.0.0? Or has it been 100% broken since v2.0.0? (i.e., it can't break backwards compatibility because there's no possible chance that anyone has been using it because it has been completely broken)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll fix that wording.

@artpol84 could you test 2.0.0 release to see if MLNX sees the XRC issue there as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jsquyres let's clarify. its not completely broken. it works for me on my connectx5 MOFED 4.0.0 cluster. It just doesn't work for Mellanox.

@hppritcha
Copy link
Member Author

@artpol84 could you test whether XRC support is broken on your cluster for the 2.0.0 release?

@jsquyres
Copy link
Member

jsquyres commented Aug 9, 2017

@hppritcha Careful about your clarification, because it's now not possible to enable XRC (via configure).

@jsquyres
Copy link
Member

@artpol84 Ping 😄

@artpol84
Copy link
Contributor

@jsquyres can you explain what is needed? Do you want me to enable XRC for v2.0.0 in Jenkins?
If so - what is the right way to configure as you pointed in #4059 (comment).

@artpol84
Copy link
Contributor

@jsquyres @hppritcha I must admit that I can't reproduce manually as well.
I tried 2.0.0 and 2.1.1 tarballs - all OK, then I checkout v2.x and revert c22a7c7 - and still all was working OK.
I'm going to enable XRC in jenkins and open a test PR with XRC commit reverted to see if this is still the case on v2.x.

@hppritcha
Copy link
Member Author

@jsquyres i removed the XRC support removed sentence since it looks like @artpol84 is busy reverting.

@hppritcha
Copy link
Member Author

okay, put the sentence back in. @artpol84 we're not taking a revert of XRC disable for 2.1.2 release.

@artpol84
Copy link
Contributor

@hppritcha it were testing PRs and at some point it seemed that XRC is ok, but more detailed analysis done by @vspetrov (#4082 (comment)) indicated that the issue can be reproduced without hcoll. So I take it back about reverting, XRC has problems.

@jsquyres
Copy link
Member

Summary from discussion on Webex 22 Aug 2017:

  • hcoll was a red herring. The issue HCOLL is causing XRC problems in v2.x #4087 is a good summary of that -- the issue is still somewhere in the openib BTL.
  • @artpol84 tested manually, and was only able to reproduce on the v2.x branch (not v2.0.x and not v3.0.x). To be clear, we don't know exactly what the issue is yet, so the fact that he wasn't able to reproduce on the other branches does not mean that the problem does not exist on the other branches.

Meaning:

  • There is definitely a problem on v2.x.
  • There is not enough proof that the problem does not occur on other branches.
  • We still need to decide what to do (e.g., in terms of disabling XRC or fixing it).

@hppritcha
Copy link
Member Author

@jsquyres I plan to rebase and merge this PR in before generating rc3 today.

@jsquyres
Copy link
Member

Ok. Summary of XRC discussion yesterday:

  • We decided that no one seems to care about XRC in the openib BTL at present.
  • Decision: we will leave it disabled. If someone wants to go in and fix it, they can re-enable it and then fix it.
  • This affects master, v2.x, and v3.0.x.

@jsquyres
Copy link
Member

Were there any additional items that needed to be added to NEWS since you created this PR 3 weeks ago?

@hppritcha
Copy link
Member Author

@jsquyres looks like there was a fix for SM BTL/CMA - #4123. Added that as well as rearranging removing items to be less depressingly located.

jsquyres
jsquyres reviewed Aug 30, 2017
View reviewed changes
NEWS Outdated
@@ -57,9 +57,44 @@ included in the vX.Y.Z section and be denoted as:
--------------------

Bug fixes/minor improvements:
- Remove IB XRC support from the OpenIB BTL due to lack of support.

- Update internal PMIx version to 1.2.3
Copy link
Member

Choose a reason for hiding this comment

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

Super minor/nit-picky: this bullet has no period (all other bullets have periods).

Copy link
Member Author

Choose a reason for hiding this comment

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

i'll fix that. Also added a blurb about NAG (ha ha).

jsquyres
jsquyres approved these changes Aug 30, 2017
View reviewed changes
Copy link
Member

@jsquyres jsquyres left a comment

Choose a reason for hiding this comment

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

Aside from the one super-picky comment, everything else is good.

Note that the date in NEWS is still August, 2017. Which is fine for today. But not Friday. 😉

@hppritcha hppritcha force-pushed the news_for_v2.1.2 branch 2 times, most recently from 2a29601 to 9538f64 Compare August 30, 2017 18:20
@hppritcha
NEWS: update news for 2.1.2 release
f1391cf 
Doing the old-fashioned thing with NEWS.
[skip ci]

Signed-off-by: Howard Pritchard <howardp@lanl.gov>
@hppritcha hppritcha merged commit cc4b5c2 into open-mpi:v2.x Aug 30, 2017
@chrissamuel chrissamuel mentioned this pull request Nov 5, 2017
Closed
@hppritcha hppritcha deleted the news_for_v2.1.2 branch May 2, 2018 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsquyres jsquyres jsquyres approved these changes

Assignees

@jsquyres jsquyres

Labels
None yet
Projects
None yet
Milestone
v2.1.2
Development

Successfully merging this pull request may close these issues.
3 participants
@hppritcha @jsquyres @artpol84