Random LLM by TrangOul · Pull Request #34 · horw/issue-title-ai

TrangOul · 2025-04-29T18:05:14Z

Closes #11

TrangOul · 2025-04-29T18:13:18Z

-from core.llm import create_ai_client
-from core.settings import Config
-from core.verbose import set_verbose
+from src.core.github_client import GitHubClient


Before this change:

ImportError while importing test module '(...)\horw-issue-title-ai\tests\test_main.py'. Hint: make sure your test modules/packages have valid Python names. Traceback: (...)\Python313\Lib\importlib\__init__.py:88: in import_module return _bootstrap._gcd_import(name[level:], package, level) tests\test_main.py:5: in <module> from src.main import open_issue_event, run, scan_issue_event src\main.py:6: in <module> from core.github_client import GitHubClient E ModuleNotFoundError: No module named 'core'

Hmm, looks good to me. How did you run the tests? I just ran:

pytest

in the project directory.

I originally ran pytest --cov=src --cov-report=xml --cov-report=html --html=test-report.html --self-contained-html in the project root dir (on Windows, if that matters). Now I've re-tested it with just pytest, after reverting the addition of src., and the same error occurs.

So it would be nice if you could say how I can reproduce it.

my logs:

(venv) horw@horw-ProArt-P16-H7606WI-H7606WI:~/PycharmProjects/issue-title-ai$ pytest ==================================================================== test session starts ===================================================================== platform linux -- Python 3.11.12, pytest-8.3.5, pluggy-1.5.0 rootdir: /home/horw/PycharmProjects/issue-title-ai configfile: pyproject.toml plugins: anyio-4.9.0, socket-0.7.0, cov-6.1.1 collected 68 items tests/test_github_client.py ............. [ 19%] tests/test_issue_service.py ........... [ 35%] tests/test_llm.py ................. [ 60%] tests/test_main.py ...... [ 69%] tests/test_settings.py ..................... [100%] ======================================================================= tests coverage ======================================================================= ______________________________________________________ coverage: platform linux, python 3.11.12-final-0 ______________________________________________________ Name Stmts Miss Branch BrPart Cover Missing ----------------------------------------------------------------------- src/core/github_client.py 48 1 8 1 96% 39 src/core/issue_service.py 46 0 12 0 100% src/core/llm.py 64 0 12 0 100% src/core/settings.py 98 0 34 1 99% 37->41 src/core/verbose.py 11 1 2 1 85% 7 src/main.py 53 2 8 2 93% 45, 90 ----------------------------------------------------------------------- TOTAL 320 4 76 5 98% Coverage HTML written to dir htmlcov Required test coverage of 80.0% reached. Total coverage: 97.73% ===================================================================== 68 passed in 0.94s ===================================================================== (venv) horw@horw-ProArt-P16-H7606WI-H7606WI:~/PycharmProjects/issue-title-ai$

TrangOul · 2025-04-29T18:52:46Z

No AI-improved title for this PR? :(
(This can be a future improvement.)

horw · 2025-04-30T02:38:23Z

No AI-improved title for this PR? :( (This can be a future improvement.)

With PRs, it seems harder to achieve this. Actually, it's okay to just look at the PR description, but as I see it, developers usually don't put much effort into the description. So, the main source of truth becomes the diffs — but that makes things more difficult (for sure, not that much more—just a few more steps):

You need to get the diffs first
Understand whether the description is good
Use the description and/or diffs to change the title

horw · 2025-04-30T07:00:16Z

Thank you for this PR! I just left a few comments. Your solution looks good!

TrangOul · 2025-04-30T07:28:49Z

+            if self.explicit_provider:
+                return self.ai_providers[self.explicit_provider]
+
+            ai_provider = random.choice(list(self.ai_providers.keys())) # nosec


@horw, I hope this nosec is OK?

Since Ruff raises this error, use # noqa: S311 to skip it.

TrangOul · 2025-04-30T08:02:31Z

Fixes #35 (quick fix, not with pathlib).

horw · 2025-04-30T06:53:31Z

+        return ai_providers
+
+    @property
+    def ai_provider(self):
+        if self.ai_providers:
+            if self.explicit_provider:
+                return self.ai_providers[self.explicit_provider]
+
+            ai_provider = random.choice(list(self.ai_providers.keys()))
+            return self.ai_providers[ai_provider]
+        raise ValueError("No LLM API key was provided. Please provide one of the following: deepseek, gemini, openai.")


Suggested change

return ai_providers

@property

def ai_provider(self):

if self.ai_providers:

if self.explicit_provider:

return self.ai_providers[self.explicit_provider]

ai_provider = random.choice(list(self.ai_providers.keys()))

return self.ai_providers[ai_provider]

raise ValueError("No LLM API key was provided. Please provide one of the following: deepseek, gemini, openai.")

if not self.ai_providers:

raise ValueError(

"No LLM API key was provided. Please provide one of the following: deepseek, gemini, openai."

)

return ai_providers

@property

def ai_provider(self):

ai_provider = random.choice(list(self.ai_providers.keys()))

return self.ai_providers[ai_provider]

How about we move the login check for No LLM ... into _get_llm_config? That way, ai_provider looks much simpler. WDYT?

Then in the validation function, ai_provider = self.ai_provider is not needed.

horw · 2025-05-01T16:38:53Z

-            "deepseek": self.deepseek_api_key,
-        }
-        self.explicit_provider = os.environ.get("INPUT_AI-PROVIDER", "").lower()
+        self._get_llm_configs()


I preferred how it was before, with the return value ai_providers, which was then assigned to self.ai_providers. The current function is named get_llm_configs, but in reality, it is doing a setting operation.

horw · 2025-05-01T16:50:35Z

+    def ai_provider(self):
+        ai_provider_name = self.ai_providers[self.explicit_provider] \
+            if self.explicit_provider \
+            else ai_provider = random.choice(list(self.ai_providers.keys())) # noqa: S311


Syntax error : else ai_provider = random...

Also, I don't think using a one-line if-else for a long expression is a good idea—it can easily be converted to a regular if-else statement.

horw · 2025-05-01T16:58:46Z

+    @property
+    def ai_provider(self):
+        ai_provider_name = self.ai_providers[self.explicit_provider] \
+            if self.explicit_provider \
+            else ai_provider = random.choice(list(self.ai_providers.keys())) # noqa: S311
+        return self.ai_providers[ai_provider_name]


Suggested change

@property

def ai_provider(self):

ai_provider_name = self.ai_providers[self.explicit_provider] \

if self.explicit_provider \

else ai_provider = random.choice(list(self.ai_providers.keys())) # noqa: S311

return self.ai_providers[ai_provider_name]

@property

def ai_provider(self):

ai_provider = random.choice(list(self.ai_providers.keys()))

return self.ai_providers[ai_provider]

WDYT about this solution? Yes, it might call random.choice a few more times, even when there's only one element, but code looks simple

TrangOul · 2025-05-16T15:36:34Z

@horw, any update? Could you run the workflows, please (or configure them to run automatically)?

horw · 2025-05-16T15:56:14Z

@horw, any update? Could you run the workflows, please (or configure them to run automatically)?

Hi! In the workflow, there are two jobs: one is the pre-commit hook, and the other is running pytest. You can easily run them locally with:

pytest

and

pre-commit run --all-files

TrangOul · 2025-05-16T18:40:08Z

Thanks @horw, I think now all the checks should pass. On my machine only the compile requirements.txt step fails (some issue with WSL), but it passes here.

TrangOul · 2025-05-21T07:23:18Z

@horw, since the tests pass now, are we OK with merging this?

horw · 2025-05-22T06:03:12Z

Hi, I will review this PR.

One note: we've changed the API, so users who already use our action will encounter a problem because the model parameter no longer exists. We can add a message to the user, such as: "The model attribute is deprecated; please use ... instead."

After you add this, I will help to fix the pre-commit issue, and then we will be ready to merge it.

TrangOul · 2025-06-02T06:23:17Z

@horw, I've added the deprecation message. Please check whether it is OK.

TrangOul · 2025-09-03T10:42:54Z

@horw, any update with this?

horw#11 random LLM

cf811b4

TrangOul commented Apr 29, 2025

View reviewed changes

Comment thread src/core/settings.py

TrangOul commented Apr 29, 2025

View reviewed changes

Comment thread tests/test_settings.py

TrangOul commented Apr 29, 2025

View reviewed changes

Comment thread src/core/settings.py

horw reviewed Apr 30, 2025

View reviewed changes

Comment thread tests/test_settings.py

fixed random seed in unit test

b45cf4a

TrangOul added 3 commits April 30, 2025 09:03

fixed f-string parsing bug occuring in outdated Python 3.11

7013786

fix for pre-commit check

4982128

suppress unsafe random lint error

37e1bc4

TrangOul commented Apr 30, 2025

View reviewed changes

fix for Windows paths horw#35

33befc2

horw reviewed Apr 30, 2025

View reviewed changes

TrangOul added 3 commits May 1, 2025 09:40

simplified AI providers getting and validation

d59c5b9

simplified getting llm

d8c8478

different models for each LLM in readme

4dadcb2

horw reviewed May 1, 2025

View reviewed changes

TrangOul added 4 commits May 1, 2025 20:14

Build fix

f4e001e

Simplified settings

295314d

Config ai_providers field fix

e151fa3

fixed DeprecationWarning in Github client

501badf

pygithub version update for new auth

a1cf763

TrangOul added 4 commits May 16, 2025 19:47

Merge remote-tracking branch 'upstream/main'

78e8e53

yamllint fixes

cf987e7

pygithub version update for new auth

b2a20f5

yamllint fixes

13011e5

TrangOul added 2 commits May 20, 2025 08:48

Merge remote-tracking branch 'upstream/main'

f12fc78

fix for compile requirements.txt

9ba5c1a

deprecation warning for 'model' param

9b97b64

Conversation

TrangOul commented Apr 29, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

horw Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TrangOul Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

horw Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TrangOul commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

horw commented Apr 30, 2025

Uh oh!

Uh oh!

horw commented Apr 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TrangOul commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TrangOul commented May 16, 2025

Uh oh!

horw commented May 16, 2025

Uh oh!

TrangOul commented May 16, 2025

Uh oh!

TrangOul commented May 21, 2025

Uh oh!

horw commented May 22, 2025

Uh oh!

TrangOul commented Jun 2, 2025

Uh oh!

TrangOul commented Sep 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

horw Apr 30, 2025 •

edited

Loading

TrangOul Apr 30, 2025 •

edited

Loading

horw Apr 30, 2025 •

edited

Loading

TrangOul commented Apr 29, 2025 •

edited

Loading

TrangOul commented Apr 30, 2025 •

edited

Loading