Skip to content

feat: separate out remaining dependencies and improve tests#789

Merged
jakelorocco merged 6 commits intomainfrom
jal/improve-dependency-isolation-tests
Apr 16, 2026
Merged

feat: separate out remaining dependencies and improve tests#789
jakelorocco merged 6 commits intomainfrom
jal/improve-dependency-isolation-tests

Conversation

@jakelorocco
Copy link
Copy Markdown
Contributor

@jakelorocco jakelorocco commented Apr 6, 2026

Misc PR

Type of PR

  • Bug Fix
  • New Feature
  • Documentation
  • Other

Description

We no longer need to explicitly version click. Typer is better about its click versioning in its own pyproject but the error is also gone.

Finishes up the dependency isolation tests; splits cli into it's own group; and fixes up import errors and dependency separation.

CI doesn't run the dependency isolation tests; ensured they pass locally.

All nice import error messages function like our current ones. The m cli works this way as well:

(mellea) ~/code/mellea4 ✓ % m --help
The 'm' CLI requires extra dependencies. Please install them with: pip install "mellea[cli]"

Testing

  • Tests added to the respective file if code was changed
  • New code has 100% coverage if code as added
  • Ensure existing tests and github automation passes (a maintainer will kick off the github automation when the rest of the PR is populated)

@jakelorocco jakelorocco linked an issue Apr 6, 2026 that may be closed by this pull request
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

The PR description has been updated. Please fill out the template for your PR to be reviewed.

@jakelorocco jakelorocco changed the title Jal/improve dependency isolation tests feat: separate out remaining dependencies and improve tests Apr 6, 2026
@github-actions github-actions bot added the enhancement New feature or request label Apr 6, 2026
@jakelorocco jakelorocco force-pushed the jal/improve-dependency-isolation-tests branch from dcc97f5 to 484a590 Compare April 6, 2026 17:13
@jakelorocco jakelorocco marked this pull request as ready for review April 6, 2026 18:21
@jakelorocco jakelorocco requested review from a team as code owners April 6, 2026 18:21
@jakelorocco
Copy link
Copy Markdown
Contributor Author

@frreiss, I believe you are getting pulled in just for the changes in mellea/formatters/granite/retrievers/util.py. This is a quality of life change but let me know if you see issues with it. Thanks!

Copy link
Copy Markdown
Contributor

@planetf1 planetf1 left a comment

Choose a reason for hiding this comment

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

initial comments

Comment thread cli/serve/commands.py
Comment thread cli/serve/app.py
Comment thread pyproject.toml
Comment thread uv.lock Outdated
@jakelorocco jakelorocco force-pushed the jal/improve-dependency-isolation-tests branch from 484a590 to 798ac76 Compare April 8, 2026 13:55
@jakelorocco jakelorocco requested a review from a team as a code owner April 8, 2026 13:55
@jakelorocco jakelorocco requested review from nrfulton and psschwei April 8, 2026 13:55
Comment thread pyproject.toml
Copy link
Copy Markdown
Contributor

@planetf1 planetf1 left a comment

Choose a reason for hiding this comment

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

Tested the CLI install (following the original comments):

Scenario Result
m (no extras) Friendly error: pip install "mellea[cli]"
m --help with [cli] Works ✓
m serve script.py with [cli] only Friendly error: pip install "mellea[server]"
m alora train ... with [cli] only Friendly error: pip install "mellea[hf]"
m alora add-readme ... with [cli] only Raw ModuleNotFoundError: No module named 'huggingface_hub'

This PR didn't touch the code in the last scenario - so opened as a followup #855

@jakelorocco jakelorocco force-pushed the jal/improve-dependency-isolation-tests branch 2 times, most recently from 2237747 to 733cc0c Compare April 15, 2026 15:45
Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com>
@psschwei
Copy link
Copy Markdown
Member

from copilot:

CI is failing in test/stdlib/components/docs/test_richdocument.py during fixture setup with:

omegaconf.errors.UnsupportedValueType: Value 'PosixPath' is not a supported primitive type
full_key: Global.model_root_dir

Root cause: Somewhere in the RichDocument/docling initialization path, Global.model_root_dir is being set as a pathlib.Path (PosixPath). OmegaConf configs only accept primitive types (str/int/float/bool/None), so it errors before tests run.

Fix: Cast Path to str before it goes into the OmegaConf dict:

from pathlib import Path

if model_root_dir is not None:
    model_root_dir = str(Path(model_root_dir))

cfg_dict["Global"]["model_root_dir"] = model_root_dir

If there may be multiple Path values, defensively stringify paths recursively before calling OmegaConf.create():

from pathlib import Path

def _stringify_paths(obj):
    if isinstance(obj, Path):
        return str(obj)
    if isinstance(obj, dict):
        return {k: _stringify_paths(v) for k, v in obj.items()}
    if isinstance(obj, list):
        return [_stringify_paths(v) for v in obj]
    return obj

cfg = OmegaConf.create(_stringify_paths(cfg_dict))

This should unblock test_richdocument_basics, test_richdocument_markdown, test_richdocument_save, and test_table.

Failing test file reference: test/stdlib/components/docs/test_richdocument.py at ref 733cc0c80cbc6590acc23b229db8210d24e66864.


Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com>
@jakelorocco jakelorocco force-pushed the jal/improve-dependency-isolation-tests branch from 733cc0c to 5f61514 Compare April 15, 2026 17:58
@jakelorocco
Copy link
Copy Markdown
Contributor Author

from copilot:

CI is failing in test/stdlib/components/docs/test_richdocument.py during fixture setup with:

...

It's an issue with package dependency requirements. Docling requires both rapidocr and omegaconf. The newest version of rapidocr is incompatible with older versions of omegaconf (when running on linux). I was able to test and setting omegaconf>= 2.1.0 works.

I'm not sure why only my PR seems to be hitting this issue though. I will see if the github action runners can succeed with this versioning change.

@psschwei
Copy link
Copy Markdown
Member

from copilot:
CI is failing in test/stdlib/components/docs/test_richdocument.py during fixture setup with:
...

It's an issue with package dependency requirements.
...

not a ringing endorsement of the built-in "explain the error" functionality is it? 🤣

@jakelorocco
Copy link
Copy Markdown
Contributor Author

Unless there are objections before tomorrow 9:00am EST, I will force merge this PR. It requires reviews from two groups that are unlikely to give reviews (since I'm usually the one reviewing for them) due to small import changes (not functional changes).

@ajbozarth
Copy link
Copy Markdown
Contributor

It requires reviews from two groups that are unlikely to give reviews

This actually came up in our sync this AM, we may want to reevaluate blocking merging of PRs in a whole dir of the project on just two reviewers, especially if those two are often the ones doing the work, thus making it block on one person in those cases.

The easiest fix would be to expand those reviewer groups, the lazier version would be to disable blocking on sub teams and stick to main maintainers

Comment thread cli/alora/intrinsic_uploader.py Outdated
Copy link
Copy Markdown
Contributor

@planetf1 planetf1 left a comment

Choose a reason for hiding this comment

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

LGTM for improving dependency tests

Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com>
@jakelorocco jakelorocco merged commit f53dc5a into main Apr 16, 2026
7 checks passed
@jakelorocco jakelorocco deleted the jal/improve-dependency-isolation-tests branch April 16, 2026 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: improve package import handling and testing

4 participants