ornladios / ADIOS2

Next generation of ADIOS developed in the Exascale Computing Program
https://adios2.readthedocs.io/en/latest/index.html
Apache License 2.0
268 stars 125 forks source link

Add some kind of warning/error when user tries to overwrite an existing BP file with a different BP version? #3594

Open caitlinross opened 1 year ago

caitlinross commented 1 year ago

Not sure if this exactly counts as a bug, but a ParaView user ran into an issue reading a BP4 file because ADIOS thought it was a BP5 file. When I debugged the issue, it turns out there wasn't actually an issue in the code, but they had an mmd.0 file in their BP4 directory. They had originally written a BP5 file, decided they wanted to use BP4 instead, but didn't delete the directory first, they just had ADIOS overwrite that. Except the mmd.0 file lingered, making ADIOS think it's a BP5 file when opening and then failing later.

So should ADIOS be doing something when writing a BP file to a filename that already exists (at least when the BP version is different), like wipe it out or give a warning about what they're doing? Or perhaps a better error message at read so the user can figure out what the problem is more easily?

eisenhauer commented 1 year ago

I'd say it counts as a bug. This wasn't really an issue with BP3, but both BP4 and BP5 create directories of the appropriate name and just overwrite what's there, but don't take care to delete what's not going to be overwritten. It probably makes sense for the CreateDirectory call to actually remove and recreate the directory if it exists. However, CreateDirectory is implemented by the KWsys thirdparty inclusion, presumably so that it's portable across platforms. It looks like maybe SystemTools::RemoveFile() will call RemoveDirectory or unlink for directories, but I suspect that you really need to do a recursive descent to empty the directory before this will work. Offhand I don't see that implementation in KWsys, but maybe Kitware has it somewhere or it can be added?

pnorbert commented 1 year ago

It was the simplest way to "determine" if a dataset is BP5 or BP4 if mmd.0 existed or not. This bug shows that it is not correct though. The correct way to determine the version is reading byte 37 in the md.idx which is either 5 or 4 for these two file formats. helper::BPVersion() needs to be rewritten in the style of helper::IsHDF5File()

dmitry-ganyushin commented 1 year ago

I would also add a git commit hash of the build into BP4/BP4 files and into the command output of 'bpls -h'" in order to track faster what version of ADIOS was used.

eisenhauer commented 1 year ago

I would also add a git commit hash of the build into BP4/BP4 files and into the command output of 'bpls -h'" in order to track faster what version of ADIOS was used.

Might be reasonable, but not relevant to this issue. This is about what engine is used, not what version of the library was involved...