Adjust the way of determining FileHandle's compatibility mode for sync and async I/O to improve code readability #608

kingcrimsontianyu · 2025-01-31T03:44:31Z

This PR improves the readability of compatibility mode handling.

The current way of determining FileHandle's compatibility mode is somewhat complicated and unintuitive. The data member _compat_mode accompanied by some utility functions more or less combines 3 different things into one:

The initially requested compat mode (ON/OFF/AUTO)
The capability of performing synchronous cuFile I/O (bool)
The capability of performing asynchronous cufile I/O (bool)

The disadvantages include:

FileHandle::is_compat_mode_preferred() always derives the preferred compat mode on the fly as opposed to getting an already determined value.
FileHandle::is_compat_mode_preferred_for_async(CompatMode) is potentially throwing, which is asymmetric to is_compat_mode_preferred(). Also when the compat mode is OFF, it has to invoke is_stream_api_available() and config_path() on each pass instead of getting an already determined value.
There is no way to retrieve what the original requested compat mode is.

These add to cognitive burden when rereading the source to introduce new features to FileHandle. This PR attempts to improve the logic by making it concise and crystal clear.

This PR also fixes a line number bug in error handling.

This PR is breaking in that the rarely used public functions to query the compat mode data in the FileHandle are removed. These data are instead queryable via the new CompatModeManager class.

copy-pr-bot · 2025-01-31T03:44:35Z

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

kingcrimsontianyu · 2025-01-31T03:44:45Z

/ok to test

kingcrimsontianyu · 2025-01-31T04:40:42Z

/ok to test

madsbk

I think this is a good idea but I am wondering if we should go a step further and implement a ResolveCompatMode class that encapsulate ALL compat-mode logic?

kingcrimsontianyu · 2025-01-31T14:59:18Z

I image that ResolveCompatMode would take over FileHandle's constructor's responsibility: (1) to determine the values of _is_compat_mode_preferred and _is_compat_mode_preferred_for_async, and (2) to potentially throw exceptions if the cuFile path is not possible under the condition _compat_mode_requested==OFF. However, the side effect, i.e. returning the file descriptors, seems to be tightly coupled with these steps. I'm not quite sure how to separate them out to let ResolveCompatMode solely focus on the compat-mode logic. Otherwise, it feels the code is simply moved from the constructor to a separate function. Please let me know what you think of this!

madsbk · 2025-01-31T15:46:30Z

Right, ResolveCompatMode would need something like a .set_cufile_init_failure() method.

I am not sure, it just think it would simplify the code to have all the compat infer code in one place, or maybe not :)

kingcrimsontianyu · 2025-01-31T21:15:58Z

I see. ~~We may add this in a future PR once the implementation details become more clear to us~~. I've had an idea how to implement this. Will update the PR next week.
Meanwhile, I'm increasing the scope of this PR by addressing #607. The FileHandle constructor is now slightly leaner.

kingcrimsontianyu · 2025-01-31T21:16:14Z

/ok to test

bdice · 2025-02-02T16:08:30Z

cpp/include/kvikio/compat_mode.hpp

+// Enable documentation of the enum.
+/**
+ * @file
+ */


Should #pragma once and the includes go before this documentation block? Does this documentation block need to have any content?

I think the order of this command and #pragma once does not matter here. The block doesn't need any content. The presence of @file alone enables the enum doc generation.

But on a second thought, perhaps a better option is to simply document the namespace kvikio in the defaults.hpp. This would automatically generate docs for all namespace scope entities. What do you think @bdice ?

I think we try to keep #pragma once at the top of the file. Your proposal to automatically generate docs sounds reasonable on the surface but I am not familiar enough with how Doxygen works to have strong opinions here.

bdice · 2025-02-02T16:09:47Z

cpp/include/kvikio/compat_mode.hpp

+enum class CompatMode : uint8_t {
+  OFF,  ///< Enforce cuFile I/O. GDS will be activated if the system requirements for cuFile are met
+        ///< and cuFile is properly configured. However, if the system is not suited for cuFile, I/O
+        ///< operations under the OFF option may error out, crash or hang.


Errors are acceptable. Is there a way to prevent crashes and hangs? It seems like we should be able to error in any situation where we would have otherwise fallen back.

Thanks for noticing this. I meant to say "unspecified behavior" and wanted to give a couple of examples, but it looks like returning the non-success error code is my general observation. So I'll remove the "crash or hang" part from here and the doc.

bdice · 2025-02-02T16:12:26Z

cpp/include/kvikio/file_handle.hpp

 #include <kvikio/parallel_operation.hpp>
 #include <kvikio/posix_io.hpp>
 #include <kvikio/shim/cufile.hpp>
 #include <kvikio/stream.hpp>
 #include <kvikio/utils.hpp>
+#include "kvikio/shim/cufile_h_wrapper.hpp"


Suggested change

#include "kvikio/shim/cufile_h_wrapper.hpp"

#include <kvikio/shim/cufile_h_wrapper.hpp>

Thanks. Done.

bdice · 2025-02-02T16:13:07Z

cpp/include/kvikio/file_utils.hpp

+#include <optional>
+#include <string>
+
+#include "kvikio/shim/cufile_h_wrapper.hpp"


Suggested change

#include "kvikio/shim/cufile_h_wrapper.hpp"

#include <kvikio/shim/cufile_h_wrapper.hpp>

bdice · 2025-02-02T16:17:33Z

cpp/src/file_handle.cpp


 #include <kvikio/defaults.hpp>
 #include <kvikio/file_handle.hpp>
+#include <kvikio/file_utils.hpp>
+#include "kvikio/compat_mode.hpp"


Suggested change

#include "kvikio/compat_mode.hpp"

#include <kvikio/compat_mode.hpp>

madsbk · 2025-02-03T07:16:37Z

cpp/include/kvikio/compat_mode.hpp

+/**
+ * @brief Parse a string into a CompatMode enum.
+ *
+ * @param compat_mode_str Compatibility mode in string format(case-insensitive). Valid values


Suggested change

* @param compat_mode_str Compatibility mode in string format(case-insensitive). Valid values

* @param compat_mode_str Compatibility mode in string format (case-insensitive). Valid values

Thanks. Done.

madsbk · 2025-02-03T07:17:25Z

cpp/include/kvikio/compat_mode.hpp

+CompatMode parse_compat_mode_str(std::string_view compat_mode_str);
+
+}  // namespace detail
+


Docs of CompatModeManager would be good

Sure! Will do!

cpp/include/kvikio/defaults.hpp

madsbk · 2025-02-03T07:20:55Z

cpp/include/kvikio/compat_mode.hpp

+  bool is_compat_mode_preferred(CompatMode compat_mode) noexcept;
+
+  std::tuple<FileWrapper, FileWrapper, CUFileHandleWrapper, bool, bool>
+  resolve_compat_mode_for_file(std::string const& file_path,


Docs of resolve_compat_mode_for_file would be good

madsbk · 2025-02-03T07:29:05Z

cpp/src/file_handle.cpp

+           _is_compat_mode_preferred,
+           _is_compat_mode_preferred_for_async) =


What about including them in the state of CompatModeManager ?
And replace compat_mode_requested, is_compat_mode_preferred, is_compat_mode_preferred_for_async. with a const reference to a CompatModeManager instance?

Good idea! Implemented.

To clarify, my current implementation is to make the defaults singleton hold a CompatModeManager as its data member, and likewise make each FileHandle hold a per-instance CompatModeManager.

cpp/include/kvikio/error.hpp

madsbk

@kingcrimsontianyu If it is not too much work, I think it would help review and design discussion if you move FileWrapper to its own PR?

madsbk · 2025-02-04T07:43:10Z

cpp/include/kvikio/compat_mode.hpp

+  CompatModeManager()                                    = default;
+  ~CompatModeManager() noexcept                          = default;
+  CompatModeManager(const CompatModeManager&)            = delete;
+  CompatModeManager& operator=(const CompatModeManager&) = delete;
+  CompatModeManager(CompatModeManager&&) noexcept;
+  CompatModeManager& operator=(CompatModeManager&&) noexcept;


Suggested change

CompatModeManager() = default;

~CompatModeManager() noexcept = default;

CompatModeManager(const CompatModeManager&) = delete;

CompatModeManager& operator=(const CompatModeManager&) = delete;

CompatModeManager(CompatModeManager&&) noexcept;

CompatModeManager& operator=(CompatModeManager&&) noexcept;

Suggest to make all of this default. I think it is fine making CompatModeManager copyable.

Should we still keep the move constructor and move assignment operator explicitly defined? If made default, the moved-from object will not have proper "reset" values. The std::exchange() used in the current explicitly defined version avoids this problem.

I don't think we need to reset the values? I mean, if CompatModeManager is accessed after move, I don't think resetting its values helps?

What I was thinking of is something like this:

// std::exchange currently used in FileHandle's move constructor "resets" old_file_handle's fd to -1. auto new_file_handle = std::move(old_file_handle); // So this is valid, as fd has a specified value. query(old_file_handle.fd()); // But small whoopsy. CompatModeManager is default moved. // After moved from, all the data members of `old_file_handle._compat_mode_manager` are left unspecified. query(old_file_handle.compat_mode_requested()); query(old_file_handle.is_compat_mode_preferred()); query(old_file_handle.is_compat_mode_preferred_for_async());

Maybe I worried too much about this. 😄

I think that is a bit too defensive :)

Got it! I'll simply default everything.

madsbk · 2025-02-04T08:04:25Z

cpp/src/compat_mode.cpp

+}
+
+std::tuple<FileWrapper, FileWrapper, CUFileHandleWrapper>
+CompatModeManager::resolve_compat_mode_for_file(std::string const& file_path,


Could this be moved to the ctor? I think it is a bit surprising that it changes the state of CompatModeManager

My initial intention is to default initialize CompatModeManager, and derive the data member at a later time, i.e:

For the compat mode manager in defaults class that does not involve files, in defaults 's constructor use compat_mode_reset() to save the requested compat mode and determine the final compat mode.

For the compat mode manager in FileHandle class, use resolve_compat_mode_for_file to save the requested compat mode and determine the final compat mode for sync and async I/O (this function also returns file objects that the constructor cannot do).

Would it be better if we:

Still use CompatModeManager for the defaults class, but remove any async-related data member and method.

Have a subclass CompatModeForFileManager to include the async-related data member and method, put the "resolve_compat_mode_for_file" logic to the constructor as you suggested, and have a getter method to retrieve the generated file and handle objects?

What about keeping defaults.cpp as it was prior to this PR and then introduce CompatModeManager for file handlers?
I don't think the current defaults.cpp code it too complex. The complex starts when files are involved :)

Reverting defaults.cpp will do.

The flip side would be a slight impediment to mocking. I had intended to thoroughly mock the following code, including the is_cufile_available() function:

kvikio/cpp/src/defaults.cpp

Line 146 in b5758ec

CompatMode defaults::infer_compat_mode_if_auto(CompatMode compat_mode) noexcept

kvikio/cpp/src/defaults.cpp

Line 157 in b5758ec

bool defaults::is_compat_mode_preferred(CompatMode compat_mode) noexcept

A compromise I could imagine would be to simply have these constructor overloads:

// For defaults // File-related members are default initialized. No actual files or handles are involved. CompatModeManager(); // For FileHandle CompatModeManager(std::string const& file_path, std::string const& flags, mode_t mode, CompatMode compat_mode_requested);

That is to tolerate the dual use of CompatModeManager for defaults and FileHandle, with the slight benefit of thorough mocking.

That could also work, but I must admit, I prefer avoiding the dual use of CompatModeManager altogether.

Got it! I'll restrict CompatModeManager to file only and revert defaults.
To facilitate mocking in another PR I'll also keep infer_compat_mode_if_auto and is_compat_mode_preferred in CompatModeManager (mild code duplication).

kingcrimsontianyu · 2025-02-04T15:50:38Z

Reducing the scope of this PR as suggested. File RAII addressed in another PR: #614

kingcrimsontianyu · 2025-02-05T16:19:05Z

If I directly rebase this branch on 25.04, there would too many repeated rebase conflicts to resolve (as a result of some commits on this branch making back-and-forth changes).

But I do have a branch where I simply squash the commits of this branch and then rebase to 25.04. Should I force-push that branch to here? Or should I close this PR and open a new one for the squashed-commit branch? I asked with the hope to not lose the review comments above. Thanks for any suggestion! @bdice

madsbk · 2025-02-06T08:14:39Z

If I directly rebase this branch on 25.04, there would too many repeated rebase conflicts to resolve (as a result of some commits on this branch making back-and-forth changes).

But I do have a branch where I simply squash the commits of this branch and then rebase to 25.04. Should I force-push that branch to here? Or should I close this PR and open a new one for the squashed-commit branch? I asked with the hope to not lose the review comments above. Thanks for any suggestion! @bdice

Squash and force-push is fine.

madsbk

Looks great, I only have an one suggestion

madsbk · 2025-02-07T07:26:26Z

cpp/include/kvikio/file_handle.hpp

+
+  /**
+   * @brief Returns the initially requested compatibility mode.
+   *
+   * @return The compatibility mode initially requested.
+   */
+  CompatMode compat_mode_requested() const noexcept;


Maybe return a const reference to _compat_mode_manager instead of compat_mode_requested, is_compat_mode_preferred, and is_compat_mode_preferred_for_async. Most users will never have to use them, so I think an extra indirection is fine.

I agree. Done!
Updated the PR to reflect this slightly breaking change.

kingcrimsontianyu added improvement Improves an existing functionality non-breaking Introduces a non-breaking change c++ Affects the C++ API of KvikIO labels Jan 31, 2025

kingcrimsontianyu changed the title ~~Adjust the way of handling the compatibility mode to improve code readability~~ Adjust the way of determining FileHandle's compatibility mode for sync and async I/O to improve code readability Jan 31, 2025

kingcrimsontianyu self-assigned this Jan 31, 2025

kingcrimsontianyu marked this pull request as ready for review January 31, 2025 05:01

kingcrimsontianyu requested a review from a team as a code owner January 31, 2025 05:01

madsbk reviewed Jan 31, 2025

View reviewed changes

kingcrimsontianyu changed the title ~~Adjust the way of determining FileHandle's compatibility mode for sync and async I/O to improve code readability~~ Improve FileHandle implementation: Simplify compat mode handling + use RAII file/handle wrappers Jan 31, 2025

kingcrimsontianyu marked this pull request as draft January 31, 2025 20:50

kingcrimsontianyu marked this pull request as ready for review February 1, 2025 03:25

kingcrimsontianyu force-pushed the simplify-compat-mode-impl branch from 466301c to 4e0c305 Compare February 1, 2025 03:27

kingcrimsontianyu requested a review from a team as a code owner February 2, 2025 06:10

bdice reviewed Feb 2, 2025

View reviewed changes

madsbk reviewed Feb 3, 2025

View reviewed changes

kingcrimsontianyu commented Feb 3, 2025

View reviewed changes

cpp/include/kvikio/error.hpp Show resolved Hide resolved

madsbk reviewed Feb 4, 2025

View reviewed changes

TomAugspurger mentioned this pull request Feb 4, 2025

Retry on HTTP 50x errors #603

Open

kingcrimsontianyu changed the title ~~Improve FileHandle implementation: Simplify compat mode handling + use RAII file/handle wrappers~~ Adjust the way of determining FileHandle's compatibility mode for sync and async I/O to improve code readability Feb 4, 2025

kingcrimsontianyu force-pushed the simplify-compat-mode-impl branch from fe29675 to 1950a10 Compare February 7, 2025 04:50

madsbk reviewed Feb 7, 2025

View reviewed changes

kingcrimsontianyu added breaking Introduces a breaking change and removed non-breaking Introduces a non-breaking change labels Feb 7, 2025

kingcrimsontianyu requested a review from madsbk February 10, 2025 14:26

kingcrimsontianyu added 7 commits February 10, 2025 14:13

Solve rebase conflict

736e8c3

Make copy default

617463a

Address review comments

f00f0d4

Fix the ill-formed problem by giving compat mode manager a default ctor

be63409

Add comments. Minor adjustment

6ca9c8b

Tweak comment format

8b6ca7b

Address review comments

790c40e

kingcrimsontianyu force-pushed the simplify-compat-mode-impl branch from 2f9b2f2 to 790c40e Compare February 10, 2025 19:13

	#include "kvikio/shim/cufile_h_wrapper.hpp"
	#include <kvikio/shim/cufile_h_wrapper.hpp>

	#include "kvikio/compat_mode.hpp"
	#include <kvikio/compat_mode.hpp>

	* @param compat_mode_str Compatibility mode in string format(case-insensitive). Valid values
	* @param compat_mode_str Compatibility mode in string format (case-insensitive). Valid values

		CompatMode parse_compat_mode_str(std::string_view compat_mode_str);

		} // namespace detail

		_is_compat_mode_preferred,
		_is_compat_mode_preferred_for_async) =

Adjust the way of determining FileHandle's compatibility mode for sync and async I/O to improve code readability #608

Are you sure you want to change the base?

Adjust the way of determining FileHandle's compatibility mode for sync and async I/O to improve code readability #608

Conversation

kingcrimsontianyu commented Jan 31, 2025 • edited Loading

copy-pr-bot bot commented Jan 31, 2025

kingcrimsontianyu commented Jan 31, 2025

kingcrimsontianyu commented Jan 31, 2025

madsbk left a comment

Choose a reason for hiding this comment

kingcrimsontianyu commented Jan 31, 2025

madsbk commented Jan 31, 2025

kingcrimsontianyu commented Jan 31, 2025 • edited Loading

kingcrimsontianyu commented Jan 31, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madsbk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kingcrimsontianyu commented Feb 4, 2025

kingcrimsontianyu commented Feb 5, 2025

madsbk commented Feb 6, 2025

madsbk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kingcrimsontianyu commented Jan 31, 2025 •

edited

Loading

kingcrimsontianyu commented Jan 31, 2025 •

edited

Loading