Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change OSB Config directory from ~/.benchmark to ~/.osb #732

Merged
merged 7 commits into from
Jan 21, 2025
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions it/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@

import pytest

from osbenchmark import client, config, version
from osbenchmark import client, config, version, paths
from osbenchmark.utils import process

CONFIG_NAMES = ["in-memory-it", "os-it"]
Expand Down Expand Up @@ -150,7 +150,7 @@ def check_prerequisites():
class ConfigFile:
def __init__(self, config_name):
self.user_home = os.getenv("BENCHMARK_HOME", os.path.expanduser("~"))
self.benchmark_home = os.path.join(self.user_home, ".benchmark")
self.benchmark_home = paths.benchmark_confdir()
if config_name is not None:
self.config_file_name = f"benchmark-{config_name}.ini"
else:
Expand Down
31 changes: 29 additions & 2 deletions osbenchmark/paths.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,39 @@
# specific language governing permissions and limitations
# under the License.
import os

import sys
from osbenchmark.utils.io import ensure_dir, ensure_symlink

def benchmark_confdir():
default_home = os.path.expanduser("~")
return os.path.join(os.getenv("BENCHMARK_HOME", default_home), ".benchmark")
old_path = os.path.join(default_home, ".benchmark")
new_path = os.path.join(default_home, ".osb")

try:
# Ensure .benchmark directory exists
ensure_dir(old_path)

# Ensure symlink from .osb to .benchmark
ensure_symlink(old_path, new_path)

final_path = os.path.join(os.getenv("BENCHMARK_HOME", default_home), ".osb")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: could be more descriptive with the variable name, would rename final_path to benchmark_confdir_path.


return final_path

except Exception as e:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you recall what the exact exception was? Let's try to use that one instead of a general Exception.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It ended up being a FileNotFoundError, but not sure if it's worth changing to this exception since the latest commit includes a fix for that error. Lmk what you think though

Copy link
Collaborator

@IanHoang IanHoang Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if the FileNotFoundError was addressed with the fix to the Config class, we should still incorporate it as it's an error that happened and potentially could still occur in the future.

Also, it's best to not use the base Exception class since it technically violates the principle of least astonishment. Python's error handling is designed to be specific and meaningful and using base Exception goes against this design principle

error_message = (
f"Error in benchmark_confdir:\n"
f"Error type: {type(e).__name__}\n"
f"Error message: {str(e)}\n"
f"Current user: {os.getlogin()}\n"
f"Current working directory: {os.getcwd()}\n"
f"Python version: {sys.version}\n"
f"Operating system: {sys.platform}\n"
f"Permissions of {old_path}: {oct(os.stat(old_path).st_mode) if os.path.exists(old_path) else 'N/A'}\n"
f"Permissions of parent of {new_path}: {oct(os.stat(os.path.dirname(new_path)).st_mode)}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these were primarily used for debugging integ tests, we can remove them

)
print(error_message)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use console.print from utils?

raise

def benchmark_root():
return os.path.dirname(os.path.realpath(__file__))
Expand Down
28 changes: 28 additions & 0 deletions osbenchmark/utils/io.py
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,34 @@ def ensure_dir(directory, mode=0o777):
if directory:
os.makedirs(directory, mode, exist_ok=True)

def ensure_symlink(source, link_name):
"""
Ensure that a symlink exists from link_name to source.
If link_name already exists, it will be updated or replaced as necessary.

:param source: The target of the symlink
:param link_name: The path where the symlink should be created
"""
logger = logging.getLogger(__name__)
if os.path.exists(link_name):
if os.path.islink(link_name):
if os.readlink(link_name) != source:
os.remove(link_name)
os.symlink(source, link_name)
logger.info("Updated symlink: %s -> %s", link_name, source)
else:
logger.info("Symlink already correct: %s -> %s", link_name, source)
elif os.path.isdir(link_name):
shutil.rmtree(link_name)
os.symlink(source, link_name)
logger.info("Replaced directory with symlink: %s -> %s", link_name, source)
else:
os.remove(link_name)
os.symlink(source, link_name)
logger.info("Replaced file with symlink: %s -> %s", link_name, source)
else:
os.symlink(source, link_name)
logger.info("Created symlink: %s -> %s", link_name, source)

def _zipdir(source_directory, archive):
for root, _, files in os.walk(source_directory):
Expand Down
Loading