Integrating TensorFlow Protobufs #5

aharbii · 2025-01-18T07:42:16Z

This is an initial PR for review.

Adding required TensorFlow protobufs
Adding basic TensorBoard logging functionality to test the integration and setup.

eugeneo

Incredible start! Protobufs are really confusing in the start.

My comments are mostly to align our coding styles.

src/uchen/tboard/tboard_file.cc

eugeneo · 2025-01-19T01:35:02Z

src/uchen/tboard/tboard_file.h

 private:
  TBoardFile(std::ofstream file) : file_(std::move(file)) {}

+  void AddScalar(const std::string& tag, double value, uint32_t step);


Suggested change

void AddScalar(const std::string& tag, double value, uint32_t step);

void AddScalar(std::string_view tag, double value, uint32_t step);

Can also be made public, replacing RecordLoss and RecordScalar

The usage of std::string_view cannot be done in the functions deals directly with TensorFlow API as it cannot be passed as arguments for the functions. and we will end up explicitly casting to std::string preventing benefiting of string_view usage.

set_tag takes rvalue reference and copies anyway. Constructing std::string when calling it is fine. Having std::string_view in API has a lot of benefits as it allows callers use whatever string they like, not just std::string.

test/uchen/tboard/tboard_file.test.cc

eugeneo · 2025-01-19T01:37:09Z

test/uchen/tboard/tboard_file.test.cc

+  for (int i = 0; i < 10; ++i) {
+    tboard_file->RecordLoss(0.5 * i, i);
+  }
+  SUCCEED() << "Loss recorded";


How do we know it was recorded? :) Call succeeded, but it does not guarantee the data is in the file.

The testing methodology is still not yet defined, as we have several points, but I think eventually we will need a way to parse the output file for testing.

As currently the only way of validating the logic is to manually check the output tfevents file with TensorBoard viewer.

Right. We need to have a way to verify stuff like that early on.

src/uchen/tboard/tboard_file.cc

eugeneo · 2025-01-19T01:41:44Z

src/uchen/tboard/tboard_file.cc

+
+  // If the path is a directory, make the directory if it doesn't exist.
+  if (path.find('/') != std::string::npos) {
+    std::string_view directory = path.substr(0, path.find_last_of('/'));


std::filesystem::path has a lot of functionality for path manipulation.

eugeneo

Thank you! Steady progress.

eugeneo · 2025-02-05T04:12:14Z

src/uchen/tboard/tboard_file.cc

+#ifdef _WIN32
+#include <winsock2.h>
+#endif


Suggested change

#ifdef _WIN32

#include <winsock2.h>

#endif

I don't think this is being used. In general, there should be no network access in this module.

eugeneo · 2025-02-05T04:16:50Z

src/uchen/tboard/tboard_file.cc

+  std::string timestamp =
+      absl::FormatTime("%Y%m%d-%H%M%S", absl::Now(), absl::UTCTimeZone());
+  std::filesystem::path events_directory =
+      std::filesystem::path(logs_directory) / std::filesystem::path(timestamp);


This directory should come from the outside (e.g. as a function parameter). This is a library so it should be as idempotent as possible - e.g. calling the API with same arguments should yield the same result. E.g. rerunning same training (say, because we crashed) should be able to override/add to docs.

eugeneo · 2025-02-05T04:18:33Z

src/uchen/tboard/tboard_file.cc

+  std::string hostname = "localhost";
+  char hostname_buffer[HOST_NAME_MAX + 1];
+  if (gethostname(hostname_buffer, sizeof(hostname_buffer)) == 0) {
+    hostname = hostname_buffer;
+  }


Same as above, client should provide this.

eugeneo · 2025-02-05T04:21:21Z

src/uchen/tboard/tboard_file.cc

+  if (!encoded_image.ok()) {
+    LOG(ERROR) << "Failed to Add image \"" << image_name
+               << "\": " << encoded_image.status();
+    return;


If there's an error, it needs to be propagated. AddImage should return absl::Status

eugeneo · 2025-02-05T04:23:02Z

src/uchen/tboard/tboard_file.cc

+  uint32_t crc_buffer = crc32c::Crc32c((uint8_t*)buffer.data(), buffer_length);
+  crc_buffer = TBoardFile::Mask(crc_buffer);
+
+  file_.write((char*)&buffer_length, sizeof(size_t));


Suggested change

file_.write((char*)&buffer_length, sizeof(size_t));

file_.write(static_cast<char*>(&buffer_length), sizeof(size_t));

Please use appropriate C++ cast. This helps prevent UB.

Also, why the char? Char can be signed on some platforms or even be over 1 byte long.

I do not the file format, so I apologize if the comment is wrong.

eugeneo · 2025-02-05T04:27:34Z

src/uchen/tboard/tboard_file.cc

+  file_.write((char*)&crc_buffer, sizeof(uint32_t));
+}
+
+absl::StatusOr<std::string> TBoardFile::EncodeImage(std::string_view image) {


Does not need to be a member function. Move it into the anonymous namespace at the top of the file so it does not pollute the symbols table.

Let's not read image files here. Require users to provide images as std::span<std::byte>. Images may not exist in the local fs or may be stored in some special format.

eugeneo · 2025-02-05T04:28:27Z

src/uchen/tboard/tboard_file.cc

+// Motivation: it is problematic to compute the CRC of a string that
+// contains embedded CRCs.  Therefore we recommend that CRCs stored
+// somewhere (e.g., in files) should be masked before being stored.
+uint32_t TBoardFile::Mask(uint32_t crc) {


Move into anonymous namespace, does not need to be a method.

eugeneo · 2025-02-05T04:49:50Z

src/uchen/tboard/tboard_file.h

 private:
  TBoardFile(std::ofstream file) : file_(std::move(file)) {}

+  void AddScalar(const std::string& tag, double value, uint32_t step);


set_tag takes rvalue reference and copies anyway. Constructing std::string when calling it is fine. Having std::string_view in API has a lot of benefits as it allows callers use whatever string they like, not just std::string.

test/uchen/tboard/tboard_file.test.cc

eugeneo · 2025-02-05T04:51:27Z

test/uchen/tboard/tboard_file.test.cc

+  for (int i = 0; i < 10; ++i) {
+    tboard_file->RecordLoss(0.5 * i, i);
+  }
+  SUCCEED() << "Loss recorded";


Right. We need to have a way to verify stuff like that early on.

aharbii added 5 commits January 17, 2025 07:09

initial tboard logging, adding scalar logging functionality

9d612d5

adding debug configurations

50067ad

ignoring tensorboard logs output

ab0fe0b

adding record image functionality

778cabb

creating logs directory if it does not exist

315d182

eugeneo reviewed Jan 19, 2025

View reviewed changes

resolving review comments

a066ce8

eugeneo requested changes Feb 5, 2025

View reviewed changes

	void AddScalar(const std::string& tag, double value, uint32_t step);
	void AddScalar(std::string_view tag, double value, uint32_t step);

	file_.write((char*)&buffer_length, sizeof(size_t));
	file_.write(static_cast<char*>(&buffer_length), sizeof(size_t));

Integrating TensorFlow Protobufs #5

Are you sure you want to change the base?

Integrating TensorFlow Protobufs #5

Uh oh!

Conversation

aharbii commented Jan 18, 2025

Uh oh!

eugeneo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eugeneo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants