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

Rework file I/O in mesh converters for thread-safety #437

Merged
merged 2 commits into from
Sep 22, 2021
Merged

Rework file I/O in mesh converters for thread-safety #437

merged 2 commits into from
Sep 22, 2021

Conversation

dengwirda
Copy link
Contributor

@dengwirda dengwirda commented Jul 29, 2021

This PR reworks the way the C++ MpasMeshConverter.x and MpasCellCuller.x, as well as the MPAS-Tools conversion.py wrapper, handles file I/O to eliminate the need to change into local directories to account for hard-coded relative file names (e.g. graph.info).

Eliminating the os.getcwd() + os.chdir() pattern in conversion.py makes these routines thread-safe, allowing them to be called across parallel threads.

These changes write all output files generated by the mesh-converter/culler (e.g. graph.info, cellMapForward.txt, etc) into the directory defined by the output filename.

- Write graph.info files into same directory as output mesh files.

- Remove related os.chdir() pattern from python wrapper to ensure
  calls are thread-safe.
@dengwirda
Copy link
Contributor Author

To run additional tests (attached):

cd conda_package
conda env create -y -n mpas_tools_dev --file dev-spec.txt
conda activate mpas_tools_dev
python -m pip install -e .
cd ..
cd mesh_tools/mesh_conversion_tools
mkdir build
cd build
cmake -DCMAKE_INSTALL_PREFIX=$CONDA_PREFIX -DCMAKE_BUILD_TYPE=Debug ..
make
make install
python test_sphere.py
python test_planar.py

test_mesh_converter.zip

Copy link
Collaborator

@xylar xylar left a comment

Choose a reason for hiding this comment

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

For now, just requesting a few formatting fixes. I'll test this in compass tomorrow. Thanks for the nice example tests and instructions!

mesh_tools/mesh_conversion_tools/mpas_cell_culler.cpp Outdated Show resolved Hide resolved
mesh_tools/mesh_conversion_tools/mpas_cell_culler.cpp Outdated Show resolved Hide resolved
@xylar
Copy link
Collaborator

xylar commented Jul 29, 2021

@mgduda, @akturner, @matthewhoffman and @mark-petersen, this work is needed as part of working on supporting parallel tasks via Parsl. If we make meshes in parallel with threads, it turns out that we can't be doing chdir type operations because these aren't thread-safe in python.

@xylar
Copy link
Collaborator

xylar commented Jul 29, 2021

@dengwirda, could you add to the PR description that the approach for modifying the C++ code is to use the directory of the output file also as the directory for the other files (e.g. graph.info)? I think that's missing.

Copy link
Member

@matthewhoffman matthewhoffman 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 just going to approve by inspection. This makes sense and the changes look appropriate. Thank for working on this Darren.

@xylar
Copy link
Collaborator

xylar commented Jul 30, 2021

@dengwirda, I tested this in compass on both my Linux laptop and Chrysalis. I needed to build the conda package (so that conda-forge compilers were used instead of system compilers) rather than following the instructions above, which used system compilers. When I did that, I got bit-for-bit identical results with the ocean nightly test suite for all tests.

I will approve as soon as the formatting issues are fixed.

Copy link
Collaborator

@akturner akturner left a comment

Choose a reason for hiding this comment

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

Built and ran successfully for me

Copy link
Collaborator

@mark-petersen mark-petersen left a comment

Choose a reason for hiding this comment

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

I compiled the c++ MpasCellCuller and MpasMeshConverter on grizzly with both gnu and intel and tested them individually with a QU240 mesh. Everything appears to work fine. Thanks!

Copy link
Collaborator

@mgduda mgduda left a comment

Choose a reason for hiding this comment

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

I'll second @xylar's suggestion to fix the whitespace in mpas_cell_culler.cpp and mpas_mesh_converter.cpp.

@xylar
Copy link
Collaborator

xylar commented Aug 20, 2021

@mgduda, I went ahead and fixed the white space since it seems like @dengwirda hasn't had time. Could you give it another look to make sure you're okay with the changes otherwise? I would like to do a new release of MPAS-Tools soon and it would be nice if this were included.

Copy link
Collaborator

@xylar xylar left a comment

Choose a reason for hiding this comment

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

Approving now that white space issues are fixed.

@xylar xylar mentioned this pull request Aug 20, 2021
@xylar
Copy link
Collaborator

xylar commented Sep 22, 2021

@mgduda, is this something you might have time to try out soon? If so, the timing would be good with other fixes we want to make for another MPAS-Tools release.

@mgduda mgduda self-requested a review September 22, 2021 16:21
Copy link
Collaborator

@mgduda mgduda left a comment

Choose a reason for hiding this comment

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

Sorry about the delay!

@xylar
Copy link
Collaborator

xylar commented Sep 22, 2021

Thanks @mgduda!

@xylar xylar merged commit ff01aa7 into MPAS-Dev:master Sep 22, 2021
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.

6 participants