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

Cleanup of readers #332

Merged
merged 6 commits into from
Jan 15, 2025
Merged

Cleanup of readers #332

merged 6 commits into from
Jan 15, 2025

Conversation

HomesGH
Copy link
Contributor

@HomesGH HomesGH commented Jul 26, 2024

ASCII reader

The method readPhaseSpaceHeader of the ASCII reader is not used in the code anymore. Instead, the data of the config xml was already employed to set according values.

It seems that the method readPhaseSpaceHeader of the ASCII reader is just used for tests, see here.
Additionally, the „final-checkpoint“ is written in ASCII format.

--> We will keep the ASCII reader for now.

MPI_IOCheckpointWriter and MPI_IOReader

@cniethammer @FG-TUM
Do you know if the MPI_IOCheckpointWriter and MPI_IOReader classes are in use or working at all?

I couldn't find any reference and usage of those in the present code base.

On the history of the MPI_IOCheckpointWriter :

  • The class was introduced with this commit 4d3fa08 but didn' work correctly
  • According to this commit, it was fixed.

HomesGH added 2 commits July 26, 2024 09:48
This method was not used in the code anymore. The config xml was already used to set according values.
@HomesGH HomesGH added question Further information is requested clean-up related to the clean-up of the code and tech dept labels Jul 26, 2024
@HomesGH HomesGH marked this pull request as draft July 26, 2024 08:20
@FG-TUM
Copy link
Member

FG-TUM commented Jul 26, 2024

After grepping the whole project, it seems MPI_IOCheckpointWriter is not used at all, and MPI_IOReader is only referenced from commented-out code once in PressureGradient, so it is also unused. Both are not registered as plugins, hence, they will probably not work at the moment.

I'd be in favor of removing them!

@HomesGH HomesGH requested a review from FG-TUM December 19, 2024 12:05
@HomesGH HomesGH marked this pull request as ready for review December 19, 2024 12:06
@HomesGH HomesGH removed the question Further information is requested label Dec 19, 2024
@cniethammer
Copy link
Contributor

cniethammer commented Dec 20, 2024

Regarding the MPI_IOCheckpointWriter and MPI_IOReader:
I am not sure if they are working at the moment - but I will try and test.

I think it would be important to have a high-performance I/O plugin in ls1 for checkpointing. MPI functionality can bypass any type-conversions and allows optimal usage of parallel file systems like, e.g., Lustre. So it would be good to keep this.
Especially in the context of WindHPC, this would be important where we might have workflows with short runtimes.

Note: I would also take over the maintainer role for this plugin :)

@HomesGH
Copy link
Contributor Author

HomesGH commented Dec 20, 2024

Regarding the MPI_IOCheckpointWriter and MPI_IOReader: I am not sure if they are working at the moment - but I will try and test.

I think it would be important to have a high-performance I/O plugin in ls1 for checkpointing. MPI functionality can bypass any type-conversions and allows optimal usage of parallel file systems like, e.g., Lustre. So it would be good to keep this. Especially in the context of WindHPC, this would be important where we might have workflows with short runtimes.

Note: I would also take over the maintainer role for this plugin :)

I agree that it is important to have a high-performance IO in ls1. However, this should then be the default setting. From a user's perspective of view, you will not dig deep into the different plugins (btw, how many readers/writers are available in ls1? Probably a lot) and just use the default one or the ones from the examples. Those are mainly the binary checkpoint reader/writer and maybe the old ASCII reader/writer (the final-checkpoint option writes in ASCII afaik). So if we want to keep them, we should make them easily usable and set them as default.

@cniethammer cniethammer mentioned this pull request Dec 23, 2024
3 tasks
Copy link
Member

@FG-TUM FG-TUM 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 in favor of deletion.

@cniethammer what was the result of your tests and do you want to keep this or would you rather reimplement it from scratch?

@FG-TUM FG-TUM requested a review from cniethammer January 14, 2025 17:24
@cniethammer
Copy link
Contributor

Regarding the MPI_IOCheckpointWriter and the MPI_IOReader:
I had a deeper look into them over the past week. I agree to remove them for now and come up with a new implementation of an MPI-I/O-based writer for fast checkpointing.

@HomesGH HomesGH merged commit 89deb41 into master Jan 15, 2025
52 checks passed
@HomesGH HomesGH deleted the updateReaders branch January 15, 2025 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean-up related to the clean-up of the code and tech dept
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants