[Good First Issue][Bug]: ov::serialize(model) does not report when file writing fails

[mlukasze]

Context

The ChatGLM application tries to write modified model into disk: https://github.com/wenyi5608/openvino.genai/blob/fcadd455b485db5ce1330e487077c99283bb3638/llm/chatglm_cpp/chatglm.cpp#L387

When the disk is full, the ov::serialize fails and file size is smaller than expected. However, it does not show any error message for that.

What needs to be done?

To prepare proper error message to inform user about a problem

Example Pull Requests

No response

Resources

• Contribution guide - start here!
• Intel DevHub Discord channel - engage in discussions, ask questions and talk to OpenVINO developers

Contact points

@olpipi @praasz

Ticket

CVS-128429

[mishra-18]

.take

[github-actions[bot]]

Thank you for looking into this issue! Please let us know if you have any questions or require any help.

[mishra-18]

Hi I'm assuming wrapping the ov::serialize block with try-cath will be enough. Should I throw the error caught when it fails, or write my own proper error message?

[vurusovs]

@mishra-18 I propose the problem is that std::ofstream doesn't throw exceptions by default, so try/catch won't help. Let's try to set up std::ofstream with exceptions() method here: https://github.com/openvinotoolkit/openvino/blob/cd1eac8da7b1d9676ab6ccf52ff746921ec2664c/src/core/src/pass/serialize.cpp#L1232-L1237

[mishra-18]

Hi @vurusovs, Thanks for taking a look. Would this work?

try {
    std::ofstream bin_file(m_binPath, std::ios::out | std::ios::binary);
    bin_file.exceptions(std::ofstream::failbit | std::ofstream::badbit);
    OPENVINO_ASSERT(bin_file, "Can't open bin file: \"" + m_binPath + "\"");

    // create xml file 
    std::ofstream xml_file(m_xmlPath, std::ios::out);
    xml_file.exceptions(std::ofstream::failbit | std::ofstream::badbit);
    OPENVINO_ASSERT(xml_file, "Can't open xml file: \"" + m_xmlPath + "\"");

    serializeFunc(xml_file, bin_file, model, m_version);
} catch (const std::ofstream::failure& e) {
    // Handle file stream errors
    std::cerr << "Exception opening/writing file. Not Enough Space in disk: " << e.what() << '\n';
    std::remove(m_xmlPath.c_str());
    std::remove(m_binPath.c_str());
    throw; 
} catch (const ov::AssertFailure& e) {
    // Handle other exceptions as before
    std::cerr << "OpenVINO assertion failed: " << e.what() << '\n';
    std.ext::remove(m_xmlPath.c_str());
    std.ext::remove(m_binPath.c_str());
    throw;
}

Also, I think xml_file.close(); bin_file.close(); weren't necessary (assuming the files are closed anyway once they get out of the scope).

[vurusovs]

Hi @vurusovs, Thanks for taking a look. Would this work?

try {
    std::ofstream bin_file(m_binPath, std::ios::out | std::ios::binary);
    bin_file.exceptions(std::ofstream::failbit | std::ofstream::badbit);
    OPENVINO_ASSERT(bin_file, "Can't open bin file: \"" + m_binPath + "\"");

    // create xml file 
    std::ofstream xml_file(m_xmlPath, std::ios::out);
    xml_file.exceptions(std::ofstream::failbit | std::ofstream::badbit);
    OPENVINO_ASSERT(xml_file, "Can't open xml file: \"" + m_xmlPath + "\"");

    serializeFunc(xml_file, bin_file, model, m_version);
} catch (const std::ofstream::failure& e) {
    // Handle file stream errors
    std::cerr << "Exception opening/writing file. Not Enough Space in disk: " << e.what() << '\n';
    std::remove(m_xmlPath.c_str());
    std::remove(m_binPath.c_str());
    throw; 
} catch (const ov::AssertFailure& e) {
    // Handle other exceptions as before
    std::cerr << "OpenVINO assertion failed: " << e.what() << '\n';
    std.ext::remove(m_xmlPath.c_str());
    std.ext::remove(m_binPath.c_str());
    throw;
}

Also, I think xml_file.close(); bin_file.close(); weren't necessary (assuming the files are closed anyway once they get out of the scope).

Looks fine! Let's create PR where we will discuss final solution. See you on review :smile:

[mlukasze]

PR: https://github.com/openvinotoolkit/openvino/pull/23281 Thanks @mishra-18, we will review it soon