Skip to content

Cleanup from self-review: redundant abspath, missing DeprecationWarning, logger inconsistency, eager __eq__ - #198

Merged
iskandr merged 1 commit into
masterfrom
cleanup-from-self-review
Apr 15, 2026
Merged

Cleanup from self-review: redundant abspath, missing DeprecationWarning, logger inconsistency, eager __eq__#198
iskandr merged 1 commit into
masterfrom
cleanup-from-self-review

Conversation

@iskandr

@iskandr iskandr commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

Small fixes surfaced while re-reading the past week's work with a critical eye. No behavior changes visible to normal callers; one API nicety (`DeprecationWarning`).

Changes

`mhctools/mhcflurry.py`

  • `_normalize_models_path`: drop redundant `os.path.abspath()`. `os.path.realpath()` already returns an absolute path.
  • Replace two stray `logging.info(...)` calls with `logger.info(...)`, using the module logger defined at line 25 (pre-existing inconsistency, fixed while nearby).

`mhctools/base_predictor.py`

  • `predict_peptides_dataframe` has said "Deprecated" in its docstring for a while; now emits `DeprecationWarning` with `stacklevel=2` so callers get a runtime signal to migrate to `predict_dataframe()`.

`mhctools/cli/args.py`

  • `_LazyPredictor.eq`: short-circuit on `is` identity and on non-class operands. Previously, `BigMHC == some_random_object` would eagerly `_resolve()` (importing torch) just to return `False`. Now it returns `NotImplemented` without triggering the import.

`tests/test_dataframe_schema_parity.py`

  • New test asserting `DeprecationWarning` fires.
  • Existing schema tests silence the warning locally so it doesn't pollute unrelated assertions.

Test plan

  • `pytest tests/` (non-integration subset) — 83 passed
  • `DeprecationWarning` fires and is caught via `pytest.warns`
  • Existing `_LazyPredictor` comparisons in `test_cli_registry` still pass (they compare against real class objects, which hit the `isinstance(other, type)` branch and resolve as before)

No version bump needed — these are internal refinements. Will batch with the next substantive change.

Four small improvements surfaced by an honest re-read of the past week's
work:

- _normalize_models_path: drop redundant os.path.abspath() — realpath()
  already returns an absolute path, so the extra call was dead code.
- predict_peptides_dataframe: add DeprecationWarning. The method's
  docstring has said "Deprecated" for a while; this gives callers a
  runtime signal to migrate to predict_dataframe().
- mhcflurry.py: use the module-level `logger` (already defined at line
  25) instead of the root `logging` module for the two
  "Loading MHCflurry models from ..." info messages. Pre-existing
  inconsistency, fixed while nearby.
- _LazyPredictor.__eq__: short-circuit on identity and on non-class
  operands. Previously `BigMHC == some_random_object` would eagerly
  resolve and import torch just to answer "no"; now it returns
  NotImplemented without triggering the load.

test_dataframe_schema_parity: adds a test asserting the DeprecationWarning
fires; existing tests silence it locally so the new warning doesn't
pollute unrelated assertions.
@iskandr
iskandr merged commit a9a6485 into master Apr 15, 2026
4 checks passed
@iskandr
iskandr deleted the cleanup-from-self-review branch April 15, 2026 20:33
iskandr added a commit that referenced this pull request Apr 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant