Slic open before flush #1387
Slic open before flush #1387
Conversation
…oblematic unit test again
bmhan12
force-pushed
the
feature/han12/slic_open_on_write
branch
from
54de589 to
4a3cc79
Compare
…instances" This reverts commit c9b27c8.
bmhan12
requested review from
white238,
cyrush,
kennyweiss,
rhornung67,
nselliott,
publixsubfan,
ltaylor16,
gunney1 and
agcapps
|
This PR is ready for review, confirmed with @gberg617 that it is working for their use case. |
| * | ||
| * \note The constructed std::ofstream will open and associate a file with | ||
| * the stream the first time GenericOutputStream is flushed with at | ||
| * least one buffered message. Use this constructor | ||
| * to avoid empty file creation if GenericOutputStream is not appended | ||
| * to. |
There was a problem hiding this comment.
This note (and the one at new line 82) is hard to understand. Please rephrase. Consider dropping the first sentence and changing the second to something like "Use this constructor to avoid creating an empty file if this GenericOutputStream never gets a message."
There was a problem hiding this comment.
Thanks @bmhan12 !
Please also update the RELEASE_NOTES about this change.
| std::ostringstream* oss = dynamic_cast<std::ostringstream*>(m_stream); | ||
| if(oss != nullptr) | ||
| { | ||
| std::string buffer = oss->str(); | ||
| if(!buffer.empty()) | ||
| { | ||
| delete m_stream; | ||
| m_stream = new std::ofstream(m_file_name); | ||
| (*m_stream) << buffer; | ||
| m_opened = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Interesting approach!
It took me a moment to figure out why the delete/new is necessary.
Could you please add a brief comment that we're converting from the original ostringstream to an ofstream and writing the former's contents to the latter?
|
|
||
| EXPECT_EQ(with_fmt_buffer.str(), "Test"); | ||
|
|
||
| // Cleanup generated files (not working Windows) |
There was a problem hiding this comment.
Is it known that this doesn't work on Windows?
I didn't see an issue for this -- if we don't have one, can you please create one?
There was a problem hiding this comment.
I didn't see an issue for this -- if we don't have one, can you please create one?
Opened an issue here: #1415
I didn't dive too deep into why it wasn't working on Windows for this specific case.
More generally though, there are multiple places where we call removeFile in the code but don't check the return value/check if file is really deleted.
This PR:
Explores an alternative way to create Slic log streams to files. With the current Slic approach, files must be opened/created when the stream object is constructed. If no messages are logged, the file remains empty. If there is an error and Slic tells MPI to abort, all ranks will attempt to flush, with files remaining empty if the rank could not flush before the non-collective abort call and/or no messages have been logged on that rank.
Constructors were added to
GenericOutputStream/LumberjackStream/SynchronizedStreamthat take in astd::stringthat is parsed and interpreted asstd::cout,std:cerr, or a file name. In the case of a filename, the file is not opened until slic flushes and the stream has at least one message logged. This is done inLumberjackStream/SynchronizedStreamby default constructing anstd::ofstreamobject (no file has been opened yet), and delaying theopen(filename)call until flushing. This is done inGenericOutputStreamby initially constructing anstd::ostringstreamto buffer the messages in memory, and turning thestd::ostringstreaminto astd::ofstreamfile when flushing.- While these constructors work, I am not sure if it's the best way of addressing the current lack of file support in Slic. These(Resolved. Can add getter method later tostd::stringconstructors take ownership ofstd::ostreamfrom the user, which can be limiting if you want access to both thestd::ostreamobject and the open-before-flush file behavior. In previous discussions, @gberg617 suggested adding getter methods to the in-memory messages storage (e.g. Lumberjack object in LumberjackStream, std::vector of messages in SynchronizedStream), so developers could take that information and create their own behavior. This would allow user-access to thestd::ostreamobject and the open-before-flush file behavior with some effort.std::ostreamobject if needed for access when using string constructors).