Skip to content

Conversation

@ktbolt
Copy link
Collaborator

@ktbolt ktbolt commented Dec 4, 2024

These are the changes needed to rename the svFSIplus solver to svMultiPhysics (see #305).

Lots of CMake code has been changed.

I also changed some of the Thirdparty directory names and the Code/Source files svFSI to solver and svFSILS to linear_solver.

@ktbolt ktbolt requested a review from dcodoni December 4, 2024 03:31
@mrp089
Copy link
Member

mrp089 commented Dec 4, 2024

There are a couple more files that come up with grep -irl svfsiplus --exclude-dir={build,.git}.

Those are mostly comments that can be changed with find path-to-folder -type f -exec sed -i '' 's/svFSIplus/svMultiPhysics/g' {} +.

The files tests/test_*struct.py refer to the two folders tests/cases/*struct/LV_NeoHookean_passive_genBC/genBC_svFSIplus that have svFSIplus in the name.

Copy link
Contributor

@dcodoni dcodoni left a comment

Choose a reason for hiding this comment

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

I could only find a missing / in a dockerfile, I have no other comments.

@dcodoni
Copy link
Contributor

dcodoni commented Dec 5, 2024

@ktbolt I saw you modified the dockerfiles for ubuntu20 and ubuntu22. I will make those changes locally and I will build the docker container and upload it to dockerhub.

@dcodoni
Copy link
Contributor

dcodoni commented Dec 6, 2024

@ktbolt I was checking why the tests failed. I was expecting test Ubuntu to fail since obviously the conda environment svmultiphysics does not exist at this point (I updated the simvascular/libraries:ubuntu22 on dockerhub but still the simvascular/solver:latest uses the old version of it and it will be updated after merging).
Test MacOs should have worked but I observed this error: sh: genBC_svMultiPhysicsplus/genBC.exe: No such file or directory
Also i think you should change some tolerances of the tests. I remember I increased the tolerance after using the latest MacOs.

@ktbolt
Copy link
Collaborator Author

ktbolt commented Dec 6, 2024

@dcodoni As @mrp089 mentioned struct and ustruct tests used genbc directories using the svfsiplus solver name (?). I changed these names.

@ktbolt
Copy link
Collaborator Author

ktbolt commented Dec 6, 2024

There were more files named using the svFSIplus solver name, renamed those.

@mrp089
Copy link
Member

mrp089 commented Dec 6, 2024

@dcodoni Renaming the solver is hopefully something we only do every couple of years (ideally never again). However, would it be useful to have an automated workflow for triggering a new Docker build and upload to Docker Hub from the Dockerfiles stored in this repository?

A use case would be updating, adding, or removing externals. This would also ensure that these are valid Dockerfiles that work with the current version of svMultiPhysics.

@dcodoni
Copy link
Contributor

dcodoni commented Dec 6, 2024

@ktbolt @mrp089 do you think we should just have one container for ubuntu with the latest Ubuntu? Currently the workflow file is using container in my personal dockerhub account and it is looking for both Ubuntu20 and Ubuntu22.
I propose the following steps:

  • create one docker container with Ubuntu latest
  • upload the container in simvascular/libraries
  • change the workflow file to just use simvascular/libraries:ubuntu-latest

@ktbolt
Copy link
Collaborator Author

ktbolt commented Dec 6, 2024

@dcodoni If we are going to use Actions to create installers then we need multiple versions of Ubuntu.

Note that builds and tests on the latest Ubuntu versions could fail. For example, VTK currently does not work on Ubuntu 22 and 24.

@mrp089
Copy link
Member

mrp089 commented Dec 6, 2024

We could do an "officially supported" hard-coded version and a -latest version. This way, we see immediately if updates break things. Also, we already have the latest version running when support is dropped for a previous version.

4C has an action to check if a Docker rebuild is required. I think they do that by comparing the hashes of their dependencies.

@ktbolt
Copy link
Collaborator Author

ktbolt commented Dec 6, 2024

@mrp089 Sounds like a good strategy.

@dcodoni
Copy link
Contributor

dcodoni commented Dec 7, 2024

@mrp089 this is a good idea! It requires some thinking: we currently build at each PR a container that as a BASE imports simvascular:libraries-ubuntu22. If we want an ubuntu-latest base library, this cant be built at each PR it would require an enormous amount of time, so we need to think who will build it and when to build it.

Anyway for now I uploaded:

  • simvascular/libraries:ubuntu20
  • simvascular/libraries:ubuntu22
    @ktbolt try to update the test_ubuntu.yml in the container section replacing container: dcodoni/lib:${{ matrix.image }} with container: simvascular/libraries:${{ matrix.image }}

Hopefully this will allow us to run the tests succesfully!

@dcodoni dcodoni merged commit 3fa507c into SimVascular:main Dec 9, 2024
5 checks passed
@ktbolt ktbolt deleted the Rename-svFSIplus-to-svMultiPhyisics_305 branch January 8, 2025 20:24
divyaadil23 pushed a commit to divyaadil23/svFSIplus that referenced this pull request Jan 29, 2025
* Change file name.

* Change names for high-level CMake for project name, executable, etc.

* Chane Docker files.

* Change documention files.

* Change thirdparty parametis_svfsi to parmetis_internal.

* Rename thridparty gklib_svfsi to gklib_internal.

* change thridparty metis_svfsi to metis_internal

* Rename package Booleans using svFSI.

* Add CMake comments to try to explain how all of the thirdparty works.

* Rename svFSI and svFSILS source code directories.

* Change tests Python scripts to use vMultiPhysics, change svFSIplus.xml to solver.xml, replace svFSIFile with svMultiPhysicsFile in xml files.

* Change names in workflow yaml files.

* Update README with name change.

* Fix some missed name changes.

* Remove some testing artifacts.

* Add missing slash.

* Rename some directories used by genbc.

* Rename some solver input parameter files named with the solver name.

* Change more hidden svFSIplus names.

* Modify container section.
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