Initial lint run

[edmondop]

No description provided.

[nqn]

I'm so sorry we lost track of this @edmondop ! How did you run this style fix? Would love to rerun and get us up to speed

[edmondop]

I'm so sorry we lost track of this @edmondop ! How did you run this style fix? Would love to rerun and get us up to speed

I can rebase and reimplement the fix if you feel like , shouldn't be hard

[nqn]

Thanks Edmondo Niklas

…

On Thu, Nov 16 2023 at 1:34 PM, Edmondo Porcu ***@***.***> wrote: I'm so sorry we lost track of this @edmondop <https://github.com/edmondop> ! How did you run this style fix? Would love to rerun and get us up to speed I can rebase and reimplement the fix if you feel like , shouldn't be hard — Reply to this email directly, view it on GitHub <#54 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAG3CXSEGMS5LEHP45AYRJDYE2BGDAVCNFSM6AAAAAA4FHIDV2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJVGM2TAMRQGE> . You are receiving this because you commented.Message ID: ***@***.***>

[edmondop]

There are a lot of improvement that could be done, such as removing unused imports, getting the code length to 120, removing unused variables, etc. Can you check the current state and confirm you agree with the direction? There is still lot of work to do @nqn

[wenzhe-log10]

@edmondop thanks for contributing!
I noticed you used flake8. There's a newer tool called ruff, which has been adopted by some Python libraries like Hugging Face, OpenAI, and Anthropic. It's often used alongside black and isort for formatting. E.g.

• openai (anthropic has identical code): https://github.com/openai/openai-python/blob/90aa5eb3ed6b92d9a1de89c0ee063f4768f92256/pyproject.toml#L64-L76
• HuggingFace: [Styling] stylify using ruff huggingface/transformers#27144

I think it would be beneficial for us to adopt a similar approach. Have you had any experience with ruff? Would you be open to giving it a try and updating the PR with it?

[edmondop]

@edmondop thanks for contributing! I noticed you used flake8. There's a newer tool called ruff, which has been adopted by some Python libraries like Hugging Face, OpenAI, and Anthropic. It's often used alongside black and isort for formatting. E.g.

• openai (anthropic has identical code): openai/openai-python@90aa5eb/pyproject.toml#L64-L76
• HuggingFace: [Styling] stylify using ruff huggingface/transformers#27144

I think it would be beneficial for us to adopt a similar approach. Have you had any experience with ruff? Would you be open to giving it a try and updating the PR with it?

I like the idea @wenzhe-log10 ! It looks there's quite a bit of work to be done to bring the code in line with styleguides, I just wanted to check there's buy-in to do this before I proceed further

[wenzhe-log10]

@edmondop That's great! @nqn and I have discussed and we are on board to proceed with the update. I would recommend starting with something similar to the current openai-python practice.

Greatly appreciate your initiative. Let us know if you need any assistance.

[edmondop]

@edmondop That's great! @nqn and I have discussed and we are on board to proceed with the update. I would recommend starting with something similar to the current openai-python practice.

Greatly appreciate your initiative. Let us know if you need any assistance.

Almost done, there's a lot of work to do with implicits optionals that are forbidden under PEP 484 and function/types , but I am getting there. One of the biggest question I have is about models. Take for example this code:

https://github.com/log10-io/log10/blob/main/log10/llm.py#L113

In this point, the code correctly guards against log10_config not being set. Few lines below it doesn't:

https://github.com/log10-io/log10/blob/main/log10/llm.py#L137

In order to get mypy happy, I need to align the behavior. Is it possible for hparams to be not set, and for log10 config to be not set? @wenzhe-log10

[wenzhe-log10]

Is it possible for hparams to be not set, and for log10 config to be not set?

@edmondop yes, both could be not set.

[edmondop]

Is it possible for hparams to be not set, and for log10 config to be not set?

@edmondop yes, both could be not set.

then you agree that L137 for example as above should check for it to be set. What's the expected behavior when the config is not set? NOOP? raise an Exception?

[wenzhe-log10]

Generally speaking, it is NOOP, like return None here.

log10/log10/llm.py

Lines 175 to 176 in 38f62f8

	if not self.log10_config:
	return None

Do you see many places need adding the guard? In log10/llm.py, I see it's missing for func last_completion_url and we could return None.

[edmondop]

@wenzhe-log10 after spending 2/3 hours there are still several hundreds of errors with py-right. It's probably easier to rewrite from scratch the code rather than fixing design problems and usage of problematic patterns, what do you think?

[wenzhe-log10]

@edmondop thanks for the effort. I'm curious to see the errors. Could you please tell me the steps to reproduce the errors?

[edmondop]

@edmondop thanks for the effort. I'm curious to see the errors. Could you please tell me the steps to reproduce the errors?

You can checkout the code and run make lint or make lint-pyright for example

[wenzhe-log10]

@edmondop I didn't find lint-pyright in makefile. Wonder if the branch has your latest change. The last commit is from 5 days ago.

[edmondop]

@edmondop I didn't find lint-pyright in makefile. Wonder if the branch has your latest change. The last commit is from 5 days ago.

Apologies, got a rejected push. Can you try now?

[wenzhe-log10]

See your latest code now. Thank you.

[wenzhe-log10]

@edmondop I used ruff for lint and format and created a PR #75. Currently opt out the pyright stuff. Do you mind if I close this PR? thanks.