Further improve error and warning infrastructure #2
Conversation
|
This is a benchmark review for experiment This pull request was cloned from Experiment configurationreview_config:
# User configuration for the review
# - benchmark - use the user config from the benchmark reviews
# - <value> - use the value directly
user_review_config:
enable_ai_review: true
enable_rule_comments: false
enable_complexity_comments: benchmark
enable_security_comments: benchmark
enable_tests_comments: benchmark
enable_comment_suggestions: benchmark
enable_functionality_review: benchmark
enable_pull_request_summary: benchmark
enable_review_guide: benchmark
enable_approvals: true
ai_review_config:
# The model responses to use for the experiment
# - benchmark - use the model responses from the benchmark reviews
# - llm - call the language model to generate responses
model_responses:
comments_model: benchmark
comment_validation_model: benchmark
comment_suggestion_model: benchmark
complexity_model: benchmark
docstrings_model: benchmark
functionality_model: benchmark
security_model: benchmark
tests_model: benchmark
# The pull request dataset to run the experiment on
pull_request_dataset:
- https://github.com/mraniki/MyLLM/pull/560
- https://github.com/Bilbottom/billiam-database/pull/92
- https://github.com/Bilbottom/loan-calcs/pull/1
- https://github.com/Bilbottom/sql-learning-materials/pull/28
- https://github.com/flet-dev/flet/pull/3522
- https://github.com/flet-dev/flet/pull/3523
- https://github.com/glenn-jocher/glenn-jocher/pull/15
- https://github.com/hbmartin/podcast-transcript-tools/pull/14
- https://github.com/joethesaint/Vue_0x01-Crash_Course/pull/9
- https://github.com/joethesaint/Vue_0x01-Crash_Course/pull/10
- https://github.com/pgarwol/medichain/pull/31
- https://github.com/pgarwol/medichain/pull/32
- https://github.com/pgarwol/medichain/pull/33
- https://github.com/pgarwol/medichain/pull/34
- https://github.com/MicroPanda123/bucket-webring/pull/1
- https://github.com/supabase-community/auth-py/pull/531
- https://github.com/supabase-community/auth-py/pull/532
- https://github.com/dreamerminsk/tasked/pull/59
- https://github.com/leaver2000/nzthermo/pull/23
- https://github.com/dreamerminsk/tasked/pull/58
- https://github.com/vicb/flyxc/pull/256
- https://github.com/CongLeSolutionX/conglesolutionx.github.io/pull/6
- https://github.com/CongLeSolutionX/conglesolutionx.github.io/pull/7
- https://github.com/CongLeSolutionX/conglesolutionx.github.io/pull/8
- https://github.com/CongLeSolutionX/conglesolutionx.github.io/pull/9
- https://github.com/CongLeSolutionX/conglesolutionx.github.io/pull/10
- https://github.com/CongLeSolutionX/conglesolutionx.github.io/pull/11
- https://github.com/Advik-B/LittleThings/pull/3
- https://github.com/theochem/iodata/pull/349
- https://github.com/theochem/iodata/pull/351
- https://github.com/theochem/iodata/pull/350
- https://github.com/iptux-src/iptux/pull/606
- https://github.com/iptux-src/iptux/pull/607
- https://github.com/EvickaStudio/PuzzelMVC/pull/1
- https://github.com/EvickaStudio/PuzzelMVC/pull/2
- https://github.com/EvickaStudio/PuzzelMVC/pull/3
- https://github.com/TeKrop/overfast-api/pull/145
- https://github.com/EvickaStudio/portfolio-website/pull/2
- https://github.com/EvickaStudio/portfolio-website/pull/3
- https://github.com/SilentDemonSD/WZML-X/pull/264
- https://github.com/R4vante/Physixed/pull/20
- https://github.com/R4vante/Physixed/pull/24
- https://github.com/bengosney/cerberus/pull/936
- https://github.com/yazdipour/OmnivoreQL/pull/27
- https://github.com/yazdipour/OmnivoreQL/pull/28
- https://github.com/gdsfactory/gdsfactory/pull/2904
- https://github.com/gdsfactory/kfactory/pull/407
- https://github.com/gdsfactory/cspdk/pull/47
- https://github.com/gdsfactory/gdsfactory/pull/2908
- https://github.com/gdsfactory/cspdk/pull/48
review_comment_labels:
- label: correct
question: Is this comment correct?
- label: helpful
question: Is this comment helpful?
- label: comment-type
question: Is the comment type correct?
- label: comment-area
question: Is the comment area correct?
- label: llm-test
question: |
What type of LLM test could this comment become?
- 👍 - this comment is really good/important and we should always make it
- 👎 - this comment is really bad and we should never make it
- no reaction - don't turn this comment into an LLM test
# Benchmark reviews generated by running
# python -m scripts.experiment benchmark <experiment_name>
benchmark_reviews: []
|
Reviewer's Guide by SourceryThis pull request improves the error and warning infrastructure by standardizing messages to include file and line number information, introducing base classes for file-related errors and warnings, and refactoring functions to handle warnings more effectively. It also updates the prepare_dump function signatures and adds tests for the new warning behaviors. File-Level Changes
Tips
|
There was a problem hiding this comment.
Hey @sourcery-ai-experiments-bot - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟡 Complexity: 2 issues found
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| # --- Renormalized contractions | ||
| normed_obasis = _fix_obasis_normalize_contractions(obasis) | ||
| if _is_normalized_properly(normed_obasis, atcoords, coeffsa, coeffsb, norm_threshold): | ||
| lit.warn("Corrected for unnormalized contractions in Molden/MKL file.") | ||
| warn( | ||
| LoadWarning( |
There was a problem hiding this comment.
suggestion: Consider consolidating warning logic.
The logic for issuing warnings is repeated in several places. Consider consolidating this logic into a helper function to reduce code duplication and improve maintainability.
There was a problem hiding this comment.
What type of LLM test could this comment become?
- 👍 - this comment is really good/important and we should always make it
- 👎 - this comment is really bad and we should never make it
- no reaction - don't turn this comment into an LLM test
| warn( | ||
| LoadWarning("Using the atom name in the PDB file to guess the chemical element.", lit), | ||
| stacklevel=2, | ||
| ) | ||
| if atnum is None: | ||
| atnum = 0 | ||
| lit.warn(f"Failed to determine the atomic number. atname='{atname}' symbol='{symbol}'") | ||
| warn( | ||
| LoadWarning( |
There was a problem hiding this comment.
suggestion: Consider adding context to warning messages.
When issuing warnings, consider adding more context to the messages to help users understand the potential impact of the issue being warned about.
There was a problem hiding this comment.
What type of LLM test could this comment become?
- 👍 - this comment is really good/important and we should always make it
- 👎 - this comment is really bad and we should never make it
- no reaction - don't turn this comment into an LLM test
| @@ -100,7 +101,7 @@ def _load_helper_atoms( | |||
| atnum = sym2num.get(symbol, sym2num.get(symbol[0], None)) | |||
| if atnum is None: | |||
| atnum = 0 | |||
| lit.warn(f"Can not convert {words[1][:2]} to elements") | |||
| warn(LoadWarning(f"Cannot interpret element symbol {words[1][:2]}", lit), stacklevel=2) | |||
There was a problem hiding this comment.
suggestion: Consider providing suggestions in warnings.
When issuing warnings about unrecognized elements or bond types, consider providing suggestions or next steps for the user to resolve the issue.
| warn(LoadWarning(f"Cannot interpret element symbol {words[1][:2]}", lit), stacklevel=2) | |
| warn(LoadWarning(f"Cannot interpret element symbol {words[1][:2]}. Please check the input for typos or refer to the documentation for supported element symbols.", lit), stacklevel=2) |
There was a problem hiding this comment.
What type of LLM test could this comment become?
- 👍 - this comment is really good/important and we should always make it
- 👎 - this comment is really bad and we should never make it
- no reaction - don't turn this comment into an LLM test
| def test_load_water_bonds_warning(): | ||
| with ( | ||
| as_file(files("iodata.test.data").joinpath("water.mol2")) as fn_mol, | ||
| pytest.warns(LoadWarning, match="Cannot interpret bond type"), |
There was a problem hiding this comment.
suggestion (testing): Add assertion for warning message
It's good practice to assert that the warning message is exactly what you expect. This ensures that any changes to the warning messages in the future will be caught by this test.
| pytest.warns(LoadWarning, match="Cannot interpret bond type"), | |
| with pytest.warns(LoadWarning) as record: | |
| mol = load_one(fn_mol) | |
| assert len(record) == 1 | |
| assert str(record[0].message) == "Cannot interpret bond type" |
There was a problem hiding this comment.
What type of LLM test could this comment become?
- 👍 - this comment is really good/important and we should always make it
- 👎 - this comment is really bad and we should never make it
- no reaction - don't turn this comment into an LLM test
|
|
||
| The error that appears in the terminal will automatically include the file name and line number. | ||
| If your code has already read the full file and encounters an error when processing the data, | ||
| you can use ``raise LoadError("Describe problem in a sentence.", lit.filename)`` instead. | ||
| This way, no line number is included in the error message. | ||
|
|
||
| Sometimes, it is possible to correct errors while reading a file. | ||
| In this case, you should warn the user that the file contains (fixable) errors: |
There was a problem hiding this comment.
suggestion (documentation): Consider using 'correctable' instead of 'fixable'.
The term 'correctable' might be more formal and precise than 'fixable'.
| In this case, you should warn the user that the file contains (fixable) errors: | |
| In this case, you should warn the user that the file contains (correctable) errors: |
There was a problem hiding this comment.
What type of LLM test could this comment become?
- 👍 - this comment is really good/important and we should always make it
- 👎 - this comment is really bad and we should never make it
- no reaction - don't turn this comment into an LLM test
| @@ -101,11 +102,13 @@ def _find_input_modules(): | |||
| INPUT_MODULES = _find_input_modules() | |||
|
|
|||
|
|
|||
| def _select_input_module(fmt: str) -> ModuleType: | |||
| def _select_input_module(filename: str, fmt: str) -> ModuleType: | |||
There was a problem hiding this comment.
issue (complexity): Consider simplifying the code by reducing the number of parameters and centralizing error handling.
The new code introduces several complexities that could be simplified. Here are some key points:
-
Increased Function Parameters: Several functions now take an additional
filenameparameter, which is only used for error messages. This increases the cognitive load on the developer, as they need to remember to pass this parameter and understand its purpose. -
Error Handling Changes: The error messages have been modified to include the filename, which is a good practice for debugging, but the way it has been implemented adds verbosity and complexity to the code. The original error messages were simpler and easier to read.
-
Decorator Usage: The introduction of the
_reissue_warningsdecorator adds another layer of abstraction. While decorators can be useful, they can also make the code harder to follow, especially for developers who are not familiar with how the decorator works. -
Redundant Code: The changes introduce redundancy in error handling. For example, the
filenameis now passed around and included in multiple error messages, which could have been handled in a more centralized manner. -
Function Signature Changes: The changes to function signatures (e.g.,
_select_input_moduleand_check_required) make the code less intuitive. The original signatures were simpler and more straightforward.
Suggested Simplifications:
-
Remove Redundant Filename Parameter: Avoid passing the
filenameparameter around unnecessarily. Use it directly where needed. -
Simplify Error Messages: Keep error messages simple and clear, while still including the filename for context.
-
Maintain Decorator Usage: Retain the
_reissue_warningsdecorator but use it in a way that doesn't add unnecessary complexity. -
Simplify Function Signatures: Keep function signatures simple and intuitive.
Here is a simplified version of the code that maintains the original simplicity while incorporating the necessary changes:
import os
import warnings
from collections.abc import Iterable, Iterator
from fnmatch import fnmatch
from importlib import import_module
from pkgutil import iter_modules
from types import ModuleType
from typing import Callable, Optional
from .iodata import IOData
from .utils import DumpError, FileFormatError, LoadError, PrepareDumpError, WriteInputError
FORMAT_MODULES = {} # Assuming this is defined somewhere in the original code
def _select_format_module(filename: str, attrname: str, fmt: Optional[str] = None) -> ModuleType:
"""Select the appropriate format module based on the filename or specified format."""
basename = os.path.basename(filename)
if fmt is None:
for format_module in FORMAT_MODULES.values():
if any(fnmatch(basename, pattern) for pattern in format_module.PATTERNS) and hasattr(format_module, attrname):
return format_module
else:
return FORMAT_MODULES[fmt]
raise FileFormatError(f"Could not find file format with feature {attrname} for file {filename}")
def _find_input_modules():
"""Return all input modules found with importlib."""
result = {}
for module_info in iter_modules(import_module("iodata.inputs").__path__):
if not module_info.ispkg:
input_module = import_module("iodata.inputs." + module_info.name)
if hasattr(input_module, "write_input"):
result[module_info.name] = input_module
return result
INPUT_MODULES = _find_input_modules()
def _select_input_module(fmt: str) -> ModuleType:
"""Find an input module."""
if fmt in INPUT_MODULES:
if not hasattr(INPUT_MODULES[fmt], "write_input"):
raise FileFormatError(f"{fmt} input module does not have write_input.")
return INPUT_MODULES[fmt]
raise FileFormatError(f"Could not find input format {fmt}.")
def _reissue_warnings(func):
"""Correct stacklevel of warnings raised in functions called deeper in IOData."""
def inner(*args, **kwargs):
"""Wrapper for func that reissues warnings."""
warning_list = []
try:
with warnings.catch_warnings(record=True) as warning_list:
result = func(*args, **kwargs)
finally:
for warning in warning_list:
warnings.warn(warning.message, warning.category, stacklevel=2)
return result
return inner
@_reissue_warnings
def load_one(filename: str, fmt: Optional[str] = None, **kwargs) -> IOData:
"""Load data from a file."""
format_module = _select_format_module(filename, "load_one", fmt)
with LineIterator(filename) as lit:
try:
return IOData(**format_module.load_one(lit, **kwargs))
except LoadError:
raise
except StopIteration as exc:
raise LoadError("File ended before all data was read.", lit) from exc
except Exception as exc:
raise LoadError("Uncaught exception while loading file.", lit) from exc
@_reissue_warnings
def load_many(filename: str, fmt: Optional[str] = None, **kwargs) -> Iterator[IOData]:
"""Load multiple IOData instances from a file."""
format_module = _select_format_module(filename, "load_many", fmt)
with LineIterator(filename) as lit:
try:
for data in format_module.load_many(lit, **kwargs):
yield IOData(**data)
except StopIteration:
return
except LoadError:
raise
except Exception as exc:
raise LoadError("Uncaught exception while loading file.", lit) from exc
def _check_required(iodata: IOData, dump_func: Callable):
"""Check that required attributes are not None before dumping to a file."""
for attr_name in dump_func.required:
if getattr(iodata, attr_name) is None:
raise PrepareDumpError(f"Required attribute {attr_name}, for format {dump_func.fmt}, is None.")
@_reissue_warnings
def dump_one(iodata: IOData, filename: str, fmt: Optional[str] = None, **kwargs):
"""Write data to a file."""
format_module = _select_format_module(filename, "dump_one", fmt)
try:
_check_required(iodata, format_module.dump_one)
if hasattr(format_module, "prepare_dump"):
format_module.prepare_dump(iodata)
except PrepareDumpError:
raise
except Exception as exc:
raise PrepareDumpError(f"{filename}: Uncaught exception while preparing for dumping to a file") from exc
with open(filename, "w") as f:
try:
format_module.dump_one(f, iodata, **kwargs)
except DumpError:
raise
except Exception as exc:
raise DumpError(f"{filename}: Uncaught exception while dumping to a file") from exc
@_reissue_warnings
def dump_many(iodatas: Iterable[IOData], filename: str, fmt: Optional[str] = None, **kwargs):
"""Write multiple IOData instances to a file."""
format_module = _select_format_module(filename, "dump_many", fmt)
iter_iodatas = iter(iodatas)
try:
first = next(iter_iodatas)
except StopIteration as exc:
raise DumpError(f"{filename}: dump_many needs at least one iodata object.") from exc
try:
_check_required(first, format_module.dump_many)
if hasattr(format_module, "prepare_dump"):
format_module.prepare_dump(first)
except PrepareDumpError:
raise
except Exception as exc:
raise PrepareDumpError(f"{filename}: Uncaught exception while preparing for dumping to a file") from exc
def checking_iterator():
"""Iterate over all iodata items, not checking the first."""
yield first
for other in iter_iodatas:
_check_required(other, format_module.dump_many)
if hasattr(format_module, "prepare_dump"):
format_module.prepare_dump(other)
yield other
with open(filename, "w") as f:
try:
format_module.dump_many(f, checking_iterator(), **kwargs)
except (PrepareDumpError, DumpError):
raise
except Exception as exc:
raise DumpError(f"{filename}: Uncaught exception while dumping to a file") from exc
@_reissue_warnings
def write_input(iodata: IOData, filename: str, fmt: str, template: Optional[str] = None, atom_line: Optional[Callable] = None, **kwargs):
"""Write input file using an instance of IOData for the specified software format."""
input_module = _select_input_module(fmt)
with open(filename, "w") as fh:
try:
input_module.write_input(fh, iodata, template, atom_line, **kwargs)
except Exception as exc:
raise WriteInputError(f"{filename}: Uncaught exception while writing an input file") from excThis approach maintains the original simplicity while incorporating the necessary changes, making the code easier to maintain and understand.
There was a problem hiding this comment.
What type of LLM test could this comment become?
- 👍 - this comment is really good/important and we should always make it
- 👎 - this comment is really bad and we should never make it
- no reaction - don't turn this comment into an LLM test
| @@ -1433,20 +1446,24 @@ def _parse_provenance( | |||
| return base_provenance | |||
|
|
|||
|
|
|||
| def prepare_dump(data: IOData): | |||
| def prepare_dump(filename: str, data: IOData): | |||
There was a problem hiding this comment.
issue (complexity): Consider simplifying the code by removing unnecessary parameters.
The current implementation introduces additional complexity by passing the file object (f) and filename around multiple functions, which are used only for error and warning messages. This increases the complexity of function signatures and calls, adds redundant information, and results in code duplication.
Consider simplifying the code by:
- Removing the
fandfilenameparameters from functions where they are not essential. - Simplifying error and warning messages to not include the file object or filename.
- Reducing code duplication to improve readability and maintainability.
This approach will maintain the original functionality while making the code cleaner and easier to follow.
There was a problem hiding this comment.
What type of LLM test could this comment become?
- 👍 - this comment is really good/important and we should always make it
- 👎 - this comment is really bad and we should never make it
- no reaction - don't turn this comment into an LLM test
Here is another PR corresponding to two tasks in theochem#191, i.e. to make all warnings and errors consistent with the previous PR that improved the
LoadErrorlogic: theochem#345. It was difficult to split this up in one PR per error or warning class because they make use of the same machinery.Main improvements:
LineIteratormethods for errors and warnings are gone.stacklevel. (For more details, see https://docs.python.org/3/library/warnings.html#warnings.warn and https://stackoverflow.com/a/71635963/494584)Related points deferred to later pull requests:
dump_*sections inCONTRIBUTING.md. This will be done after theprepare_dumpAPI has been completed.I will YOLO-merge this on Friday, June 28, unless reviewed earlier.
Summary by Sourcery
This pull request improves the error and warning infrastructure by standardizing messages to include file and line number information, introducing base classes for file-related errors and warnings, and refactoring functions to enhance error reporting. Additionally, it includes updates to tests to ensure the new error handling mechanisms work as expected.
BaseFileErrorfor all file-related errors andBaseFileWarningfor all file-related warnings.prepare_dumpfunctions across multiple modules to include the filename in error messages._reissue_warningsdecorator to correct the stack level of warnings raised in deeper functions.warnmethod fromLineIteratorand replaced it with direct calls to thewarnfunction withLoadWarning.FileFormatErrorwhen loading files with unknown extensions.