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

Revert @acxz's TBB revert #901

Merged
merged 2 commits into from
Oct 28, 2021
Merged

Revert @acxz's TBB revert #901

merged 2 commits into from
Oct 28, 2021

Conversation

ProfFan
Copy link
Collaborator

@ProfFan ProfFan commented Oct 21, 2021

I tested and didn't find any actual problem with the code by @acxz , so I think it's safe to revert.

Plus this allows building GTSAM with TBB 2021.x.

@varunagrawal
Copy link
Collaborator

If I remember correctly, wasn't the issue that a test would fail intermittently due to the lack of determinism in the concurrency? @acxz

@ProfFan
Copy link
Collaborator Author

ProfFan commented Oct 22, 2021

I think that is actually not related to this particular PR, but I could be wrong. Need more testing to be sure...

@varunagrawal
Copy link
Collaborator

I'm waiting for @acxz to make a note about the issue before I approve.

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

I'm approving this and if we encounter issues, we can handle this. :)

if ((${TBB_VERSION} VERSION_GREATER "2021.1") OR (${TBB_VERSION} VERSION_EQUAL "2021.1"))
message(FATAL_ERROR "TBB version greater than 2021.1 (oneTBB API) is not yet supported. Use an older version instead.")
endif()
# if ((${TBB_VERSION} VERSION_GREATER "2021.1") OR (${TBB_VERSION} VERSION_EQUAL "2021.1"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Kill?

@acxz
Copy link
Contributor

acxz commented Oct 27, 2021

I'm waiting for @acxz to make a note about the issue before I approve.

This was the only issue I personally encountered: #800 (comment)

The issues that came up (#820) showed that the original PR was not a problem and as such I agree with @ProfFan this is safe to merge.

As you mentioned we'll handle issues as they come up.

@ProfFan ProfFan merged commit 3755f21 into develop Oct 28, 2021
@ProfFan ProfFan deleted the fan/tbb_revert branch October 28, 2021 22:16
@varunagrawal
Copy link
Collaborator

Update on this PR: It seems to be the cause of the recent ILS errors in GTDynamics. By turning off TBB, the errors go away.

@acxz can you take a look at this and help us understand why? You're probably the best at TBB amongst us.

@acxz
Copy link
Contributor

acxz commented Nov 2, 2021

I'm afraid I don't really have the time to look at the GTDynamics issues right now.
I'll keep this in the back of my mind so that I can return to this later on in the month.

@varunagrawal
Copy link
Collaborator

This isn't a GTDynamics issue, but the GTDynamics issue is a symptom of the issue with TBB :)

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