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

[External] Update pybind11 to last version #12788

Merged
merged 5 commits into from
Oct 31, 2024

Conversation

loumalouomega
Copy link
Member

📝 Description

Update pybind11 to last version (2.13).

I am trying to compile Kratos with Intel LLVM and this update is required. See pybind/pybind11#927

🆕 Changelog

@loumalouomega loumalouomega requested a review from a team as a code owner October 25, 2024 07:57
@loumalouomega loumalouomega marked this pull request as draft October 25, 2024 11:28
@loumalouomega
Copy link
Member Author

Making draft, CI is failing, and still having link issues with Intel LLVM

@loumalouomega
Copy link
Member Author

loumalouomega commented Oct 25, 2024

Traceback (most recent call last):
  File "/__w/Kratos/Kratos/bin/Custom/kratos/tests/test_model_part.py", line 893, in test_model_part_iterators
    for subpart in model_part1.SubModelParts:
RuntimeError: return_value_policy = move, but type Kratos::PointerHashMapSet<Kratos::ModelPart, std::hash<std::string>, Kratos::ModelPart::GetModelPartName, std::shared_ptr<Kratos::ModelPart> > is neither movable nor copyable!

Clearly this requires a major refactor

@matekelemen
Copy link
Contributor

matekelemen commented Oct 26, 2024

This would be a great opportunity to store geometries in a PointerVectorSet rather than PointerHashMapSet to make them consistent with nodes, elements, conditions and MPCs ...

As far as I know, PointerHashMapSet is used exclusively to store geometries in ModelPart, and nothing else. Its interface is oddly specific for this use case, and is non-standard. Unless there's someone who enlightens me about the merits of PointerHashMapSet, I'd just remove it in favor of PointerVectorSet.

Sigh. Please ignore this, I was thinking of GeometryContainer (that wraps a PointerHashMapSet).

@RiccardoRossi
Copy link
Member

As inrecall the goal of thwt container is to allow indexing by a hash id.

The important case is that of someone not giving you an id for the geoemtries but rather one wants to identify them by the connectivity.

Having said this...i am not much into the technical.details. can someone provide more insight?

@matekelemen
Copy link
Contributor

My previous comment is irrelevant, please ignore it.

As for the error, the problem is that ModelPart::SubModelParts() returns the sub model part container by reference (instead of a shared ptr), which is not the holder type of the container. Since pybind cannot hold the container, it tries copying it, which is thankfully not allowed.

The easiest fix would be to return the sub model part container by shared ptr. However, this is impossible right now because ModelPart holds sole ownership of this container (it stores the sub model part container by value instead of shared ptr).

SubModelPartsContainerType mSubModelParts; /// The container of the submodelparts

We don't have this issue with Nodes, Elements, etc. because Mesh stores their containers by shared ptrs.

typename NodesContainerType::Pointer mpNodes;
typename PropertiesContainerType::Pointer mpProperties;
typename ElementsContainerType::Pointer mpElements;
typename ConditionsContainerType::Pointer mpConditions;
typename MasterSlaveConstraintContainerType::Pointer mpMasterSlaveConstraints;

Another fix would be to return a view over the sub model parts instead of the container itself. The view could be copied without any performance-related issues, and it would be also be safer because users wouldn't be able manipulate the container directly (e.g.: add or remove sub model parts to/from the container directly instead of the ModelPart interface).

@loumalouomega
Copy link
Member Author

ERROR: test_select_node_list (test_variable_utils.TestVariableUtils)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/__w/Kratos/Kratos/bin/Custom/kratos/tests/test_variable_utils.py", line 599, in test_select_node_list
    node_list = KratosMultiphysics.VariableUtils().SelectNodeList(KratosMultiphysics.VISCOSITY, 0.01, model_part.Nodes)
RuntimeError: return_value_policy = move, but type Kratos::PointerVectorSet<Kratos::Node, Kratos::IndexedObject, std::less<unsigned long>, std::equal_to<unsigned long>, Kratos::intrusive_ptr<Kratos::Node>, std::vector<Kratos::intrusive_ptr<Kratos::Node>, std::allocator<Kratos::intrusive_ptr<Kratos::Node> > > > is neither movable nor copyable!

We should do a view for the Nodes/Conditions/Elements as well...

@loumalouomega loumalouomega marked this pull request as ready for review October 31, 2024 07:02
@loumalouomega
Copy link
Member Author

Okay, looks like with these 2 minor changes it works now

@loumalouomega
Copy link
Member Author

BTW, this new version gives support for Eigen conversion

Copy link
Contributor

@matekelemen matekelemen left a comment

Choose a reason for hiding this comment

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

👍

This View is fine for now, but (in another PR) I'd like a proper View template we can use throughout Kratos (and not only for python bindings).

By proper I mean a standard-conforming container interface, complete with type aliases and proper iterators.

@loumalouomega loumalouomega merged commit 6b2f16a into master Oct 31, 2024
11 checks passed
@loumalouomega loumalouomega deleted the external/update-pybind11-to-last-version branch October 31, 2024 08:22
@matekelemen
Copy link
Contributor

Oh whoops, I didn't notice the automerge. Sorry.

@loumalouomega
Copy link
Member Author

Oh whoops, I didn't notice the automerge. Sorry.

Ups

@loumalouomega
Copy link
Member Author

👍

This View is fine for now, but (in another PR) I'd like a proper View template we can use throughout Kratos (and not only for python bindings).

By proper I mean a standard-conforming container interface, complete with type aliases and proper iterators.

Kokkos uses view for everything...

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.

3 participants