Skip to content

logger: remote: use lazy formatting #3178

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jan 19, 2020
Merged

logger: remote: use lazy formatting #3178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6f8ac0c
logger: support {} formatting
efiop Jan 17, 2020
c2dae4b
logger: remote: use lazy formatting
efiop Jan 17, 2020
65c0c7e
logger: state: use lazy formatting
efiop Jan 17, 2020
19acded
logger: utils: fs: use lazy formatting
efiop Jan 17, 2020
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
10 changes: 6 additions & 4 deletions dvc/logger.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,10 @@ class ColorFormatter(logging.Formatter):
}

def format(self, record):
msg = record.msg.format(*record.args) if record.args else record.msg

if record.levelname == "INFO":
return record.msg
return msg

if record.levelname == "ERROR" or record.levelname == "CRITICAL":
exception, stack_trace = self._parse_exc(record)
Expand All @@ -70,16 +72,16 @@ def format(self, record):
color=self.color_code.get(record.levelname, ""),
nc=colorama.Fore.RESET,
levelname=record.levelname,
description=self._description(record.msg, exception),
msg=record.msg,
description=self._description(msg, exception),
msg=msg,
stack_trace=stack_trace,
)

return "{color}{levelname}{nc}: {msg}".format(
color=self.color_code.get(record.levelname, ""),
nc=colorama.Fore.RESET,
levelname=record.levelname,
msg=record.msg,
msg=msg,
)

def _current_level(self):
Expand Down
80 changes: 43 additions & 37 deletions dvc/remote/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
from dvc.progress import Tqdm
from dvc.remote.slow_link_detection import slow_link_guard
from dvc.state import StateNoop
from dvc.utils import relpath, tmp_fname
from dvc.utils import tmp_fname
from dvc.utils.fs import move, makedirs
from dvc.utils.http import open_url

Expand Down Expand Up @@ -275,8 +275,10 @@ def load_dir_cache(self, checksum):
raise DirCacheError(checksum) from exc

if not isinstance(d, list):
msg = "dir cache file format error '{}' [skipping the file]"
logger.error(msg.format(relpath(path_info)))
logger.error(
"dir cache file format error '{}' [skipping the file]",
path_info,
)
Comment on lines +278 to +281
Copy link
Contributor

Choose a reason for hiding this comment

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

Still use msg maybe?

msg = "dir cache file format error '{}' [skipping the file]"
logger.error(msg, path_info)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No point in a new var, it is an old code from time when we weren't sure on formatting. We are inlining it these days, so it makes sense to inline here as well.

return []

for info in d:
Expand Down Expand Up @@ -342,36 +344,35 @@ def changed(self, path_info, checksum_info):
"""

logger.debug(
"checking if '{}'('{}') has changed.".format(
path_info, checksum_info
)
"checking if '{}'('{}') has changed.", path_info, checksum_info
)

if not self.exists(path_info):
logger.debug("'{}' doesn't exist.".format(path_info))
logger.debug("'{}' doesn't exist.", path_info)
return True

checksum = checksum_info.get(self.PARAM_CHECKSUM)
if checksum is None:
logger.debug("checksum for '{}' is missing.".format(path_info))
logger.debug("checksum for '{}' is missing.", path_info)
return True

if self.changed_cache(checksum):
logger.debug(
"cache for '{}'('{}') has changed.".format(path_info, checksum)
"cache for '{}'('{}') has changed.", path_info, checksum
)
return True

actual = self.get_checksum(path_info)
if checksum != actual:
logger.debug(
"checksum '{}'(actual '{}') for '{}' has changed.".format(
checksum, actual, path_info
)
"checksum '{}'(actual '{}') for '{}' has changed.",
checksum,
actual,
path_info,
)
Comment on lines 367 to 372
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above:

msg = "checksum '{}'(actual '{}') for '{}' has changed."
logger.debug(msg, checksum, actual, path_info)

return True

logger.debug("'{}' hasn't changed.".format(path_info))
logger.debug("'{}' hasn't changed.", path_info)
return False

def link(self, from_info, to_info):
Expand All @@ -394,8 +395,9 @@ def _try_links(self, from_info, to_info, link_types):
return

except DvcException as exc:
msg = "Cache type '{}' is not supported: {}"
logger.debug(msg.format(link_types[0], str(exc)))
logger.debug(
"Cache type '{}' is not supported: {}", link_types[0], exc
)
del link_types[0]

raise DvcException("no possible cache types left to try out.")
Expand All @@ -409,13 +411,13 @@ def _do_link(self, from_info, to_info, link_method):
if self.protected:
self.protect(to_info)

msg = "Created {}'{}': {} -> {}".format(
logger.debug(
"Created {}'{}': {} -> {}",
"protected " if self.protected else "",
self.cache_types[0],
from_info,
to_info,
)
logger.debug(msg)

def _save_file(self, path_info, checksum, save_link=True):
assert checksum
Expand Down Expand Up @@ -517,7 +519,7 @@ def save(self, path_info, checksum_info):

def _save(self, path_info, checksum):
to_info = self.checksum_to_path_info(checksum)
logger.debug("Saving '{}' to '{}'.".format(path_info, to_info))
logger.debug("Saving '{}' to '{}'.", path_info, to_info)
if self.isdir(path_info):
self._save_dir(path_info, checksum)
return
Expand All @@ -529,8 +531,9 @@ def _handle_transfer_exception(
if isinstance(exception, OSError) and exception.errno == errno.EMFILE:
raise exception

msg = "failed to {} '{}' to '{}'".format(operation, from_info, to_info)
logger.exception(msg)
logger.exception(
"failed to {} '{}' to '{}'", operation, from_info, to_info
)
return 1

def upload(self, from_info, to_info, name=None, no_progress_bar=False):
Expand All @@ -543,7 +546,7 @@ def upload(self, from_info, to_info, name=None, no_progress_bar=False):
if from_info.scheme != "local":
raise NotImplementedError

logger.debug("Uploading '{}' to '{}'".format(from_info, to_info))
logger.debug("Uploading '{}' to '{}'", from_info, to_info)

name = name or from_info.name

Expand Down Expand Up @@ -622,7 +625,7 @@ def _download_file(
):
makedirs(to_info.parent, exist_ok=True, mode=dir_mode)

logger.debug("Downloading '{}' to '{}'".format(from_info, to_info))
logger.debug("Downloading '{}' to '{}'", from_info, to_info)
name = name or to_info.name

tmp_file = tmp_fname(to_info)
Expand Down Expand Up @@ -728,9 +731,10 @@ def changed_cache_file(self, checksum):
actual = self.get_checksum(cache_info)

logger.debug(
"cache '{}' expected '{}' actual '{}'".format(
str(cache_info), checksum, actual
)
"cache '{}' expected '{}' actual '{}'",
cache_info,
checksum,
actual,
)

if not checksum or not actual:
Expand All @@ -740,7 +744,7 @@ def changed_cache_file(self, checksum):
return False

if self.exists(cache_info):
logger.warning("corrupted cache file '{}'.".format(cache_info))
logger.warning("corrupted cache file '{}'.", cache_info)
self.remove(cache_info)

return True
Expand Down Expand Up @@ -850,8 +854,9 @@ def _checkout_file(
"""The file is changed we need to checkout a new copy"""
cache_info = self.checksum_to_path_info(checksum)
if self.exists(path_info):
msg = "data '{}' exists. Removing before checkout."
logger.warning(msg.format(str(path_info)))
logger.warning(
"data '{}' exists. Removing before checkout.", path_info
)
self.safe_remove(path_info, force=force)

self.link(cache_info, path_info)
Expand Down Expand Up @@ -882,7 +887,7 @@ def _checkout_dir(

dir_info = self.get_dir_cache(checksum)

logger.debug("Linking directory '{}'.".format(path_info))
logger.debug("Linking directory '{}'.", path_info)

for entry in dir_info:
relative_path = entry[self.PARAM_RELPATH]
Expand Down Expand Up @@ -933,22 +938,24 @@ def checkout(
skip = False
if not checksum:
logger.warning(
"No checksum info found for '{}'. "
"It won't be created.".format(str(path_info))
"No checksum info found for '{}'. " "It won't be created.",
path_info,
)
self.safe_remove(path_info, force=force)
failed = path_info

elif not relink and not self.changed(path_info, checksum_info):
msg = "Data '{}' didn't change."
logger.debug(msg.format(str(path_info)))
logger.debug("Data '{}' didn't change.", path_info)
skip = True

elif self.changed_cache(
checksum, path_info=path_info, filter_info=filter_info
):
msg = "Cache '{}' not found. File '{}' won't be created."
logger.warning(msg.format(checksum, str(path_info)))
logger.warning(
"Cache '{}' not found. File '{}' won't be created.",
checksum,
path_info,
)
self.safe_remove(path_info, force=force)
failed = path_info

Expand All @@ -962,8 +969,7 @@ def checkout(
)
return failed

msg = "Checking out '{}' with cache '{}'."
logger.debug(msg.format(str(path_info), checksum))
logger.debug("Checking out '{}' with cache '{}'.", path_info, checksum)

self._checkout(
path_info, checksum, force, progress_callback, relink, filter_info
Expand Down
2 changes: 1 addition & 1 deletion dvc/rwlock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

from .exceptions import DvcException
from .lock import LockError
from .utils.fs import relpath
from .utils import relpath
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we trying to move this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Suor It is a faulty import from an incorrect place that broke once I've removed no longer used relpath re-import from fs.py.


INFO_SCHEMA = {"pid": int, "cmd": str}

Expand Down
11 changes: 6 additions & 5 deletions dvc/state.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ def _execute(self, cmd, parameters=()):

def _fetchall(self):
ret = self.cursor.fetchall()
logger.debug("fetched: {}".format(ret))
logger.debug("fetched: {}", ret)
return ret

def _to_sqlite(self, num):
Expand Down Expand Up @@ -181,11 +181,12 @@ def _prepare_db(self, empty=False):
__version__, self.VERSION, version
)
elif version < self.VERSION:
msg = (
logger.warning(
"State file version '{}' is too old. "
"Reformatting to the current version '{}'."
"Reformatting to the current version '{}'.",
version,
self.VERSION,
)
logger.warning(msg.format(version, self.VERSION))
cmd = "DROP TABLE IF EXISTS {};"
self._execute(cmd.format(self.STATE_TABLE))
self._execute(cmd.format(self.STATE_INFO_TABLE))
Expand Down Expand Up @@ -466,7 +467,7 @@ def remove_unused_links(self, used):
actual_mtime, _ = get_mtime_and_size(path, self.repo.tree)

if inode == actual_inode and mtime == actual_mtime:
logger.debug("Removing '{}' as unused link.".format(path))
logger.debug("Removing '{}' as unused link.", path)
remove(path)
unused.append(relpath)

Expand Down
8 changes: 3 additions & 5 deletions dvc/utils/fs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
from dvc.utils import dict_md5
from dvc.utils import fspath
from dvc.utils import fspath_py35
from dvc.utils import relpath


logger = logging.getLogger(__name__)
Expand All @@ -30,7 +29,7 @@ def fs_copy(src, dst):

def get_inode(path):
inode = System.inode(path)
logger.debug("Path {} inode {}".format(path, inode))
logger.debug("Path {} inode {}", path, inode)
return inode


Expand Down Expand Up @@ -131,10 +130,9 @@ def _chmod(func, p, excinfo):


def remove(path):
path = fspath_py35(path)

logger.debug("Removing '{}'".format(relpath(path)))
logger.debug("Removing '{}'", path)

path = fspath_py35(path)
try:
if os.path.isdir(path):
shutil.rmtree(path, onerror=_chmod)
Expand Down