search

alat-rights / alana-utilities

Quick utilities for interacting with LLMs.
https://utils.alana.computer
MIT License
1 stars 2 forks source link

Added precise type annotation for kwargs #13

Closed Domejko closed 6 months ago

Domejko commented 6 months ago

Regarding #2

Closes #2

alat-rights commented 6 months ago

Good work!

I think I'd prefer not to add httpx to requirements. Additional requirements makes pip install more painful for users.

If the functions still work if a user is missing httpx, I'd motion for keeping it out of requirements. Let me know if that sounds wrong.

I left some more comments :) Sorry this is a slightly complicated task.

Domejko commented 6 months ago

I think I'd prefer not to add httpx to requirements. Additional requirements makes pip install more painful for users.

If the functions still work if a user is missing httpx, I'd motion for keeping it out of requirements. Let me know if that sounds wrong.

If we remove it from requirements we won't be able to perform strict type check provided by this module on timeout argument.

alat-rights commented 6 months ago

I think we should use string-based type hint for timeout instead of adding httpx.

Domejko commented 6 months ago

I think we should use string-based type hint for timeout instead of adding httpx.

But in that case we either simplify our type hint but just removing Timeout, or we pass this hint Union[Optional[float], Tuple[Optional[float], Optional[float], Optional[float], Optional[float]], "Timeout", ] that TimeoutTypes suggests.

alat-rights commented 6 months ago

Hrm. I guess using the Union[Optional[float], Tuple[Optional[float], Optional[float], Optional[float], Optional[float]], "Timeout", ] hint looks fine, if a bit verbose.

Domejko commented 6 months ago

Ok, so I'll add now discussed changes and push them. But if ever mypy would be implemented I assume that the Timeout still maybe be causing errors.

Domejko commented 6 months ago

Discussed changes have been added so PR is ready to test and merge.

alat-rights commented 6 months ago

Good work! Sorry it might take a few days to for me to get around to fully review. Busy recently.

alat-rights commented 6 months ago

Merging.

alat-rights commented 6 months ago

Sadly this PR caused some issues:

While I was able to resolve the unit test failures easily, the typing errors were harder to resolve.

As a result, we rollback this PR for now.

Hopefully we can reimplement it in the future.

Good effort in any case! I should've done more thorough review before merging. You're also welcome to try again.

Domejko commented 6 months ago

You are running mypy on the code, or some other type checking module ?

alat-rights commented 6 months ago

Should be PyRight

Domejko commented 6 months ago

Ok, but which packages have showed you typing errors ? I'm asking because I would like to check it with this package and if possible fix it. If it was mypy then I already mentioned in comments before that mypy will probably show errors on that one.

Also for the future you should also specify this is README which type checking packages are you using so the contributor could also check code with them.

And so to be clear, since now on you will be using PyRight for type checking ?

alat-rights commented 6 months ago

Hi Domejko!

Yes you're right. Sorry about the confusion, it's a very new module and I don't have processes in place yet.

The errors are pylance errors raised on prompt.py that were resolved with 16c6b9f.

Yes. I run PyRight on my dev environment with standard type checking mode.

alat-rights commented 6 months ago

https://github.com/alat-rights/alana-utilities/issues/21