Use linting to detect code errors

[de-code]

It might be a good idea to use linting in ci to catch errors early. Common tools used are pylint and mypy.

You could ignore all of pylint's Python style recommendations and just look for errors.

e.g.:

pylint --errors-only --disable=import-error src tests

And you get a few potential errors (I filtered some out):

************* Module smolagents.agents
src/smolagents/agents.py:498:12: E1111: Assigning result of a function call, where the function has no return (assignment-from-no-return)
src/smolagents/agents.py:536:16: E1111: Assigning result of a function call, where the function has no return (assignment-from-no-return)
src/smolagents/agents.py:764:27: E0606: Possibly using variable 'observation_name' before assignment (possibly-used-before-assignment)
************* Module smolagents.gradio_ui
src/smolagents/gradio_ui.py:195:16: E1101: Instance of 'File' has no 'change' member (no-member)
src/smolagents/gradio_ui.py:201:12: E1101: Instance of 'Textbox' has no 'submit' member (no-member)
************* Module smolagents.utils
src/smolagents/utils.py:187:10: E1120: No value for argument 'logger' in constructor call (no-value-for-parameter)

The assignment-from-no-return stem from the step function not being declared as abstractmethod (you might argue that as a style).

In the following code pylint says observation_name might be used before assignment because observation_type could possibly neither be AgentImage nor AgentAudio:

                if observation_type == AgentImage:
                    observation_name = "image.png"
                elif observation_type == AgentAudio:
                    observation_name = "audio.mp3"
                # TODO: observation naming could allow for different names of same type

                self.state[observation_name] = observation

And in parse_json_tool_call the

class AgentError(Exception):
    """Base class for other agent-related exceptions"""

    def __init__(self, message, logger: AgentLogger):
        super().__init__(message)
        self.message = message
        logger.log(f"[bold red]{message}[/bold red]", level=LogLevel.ERROR)


class AgentParsingError(AgentError):
    """Exception raised for errors in parsing in the agent"""

    pass

...

def parse_json_tool_call(json_blob: str) -> Tuple[str, Union[str, None]]:
    ...
    raise AgentParsingError(error_msg)

mypy also shows a lot of warnings, some may just be incorrect type hints (which is hard to always get right if not automatically checked). Not sure if any of those indicate other coding errors.

I found adding mypy (or any linting) early can make it really much easier than adding it later on.

(Over the time I also personally warmed to many of the style recommendations)

[albertvillanova]

Please note that Python is a dynamically typed language and the use of type hints in the smolagents library is optional: we do not enforce strict static typing.

IMO, implementing strict static typing is not a priority. I think it is better to focus on adding more interesting features.

By the way, we are already using ruff as linter and formatter!

[de-code]

Please note that Python is a dynamically typed language and the use of type hints in the smolagents library is optional: we do not enforce strict static typing.

IMO, implementing strict static typing is not a priority. I think it is better to focus on adding more interesting features.

Of course, I understand that Python is dynamically typed and that is why it's called type hints. The smolagents library doesn't seem to be different to any other project in that respect.

It's like unit tests, you can see them as a chore or a helpful tool. Typing hints can be a communication tool to developers. But it can also be a helpful tool to spot errors. But they are always optional as you say and you can use it as much or little as you want.

This GitHub issue wasn't about adding type hints though. It was about using linters to help spot errors in the existing code.

There are already many typing hints within smolagents. One role of mypy here would be to for example check that they are actually correct, and sometimes they are not. That means in some instances the typing hints are confusing by documenting something that is inaccurate.

Perhaps sometimes mypy can also be annoying by trying to suggest you to add typing hints to help it do its job. But you could choose to ignore that.

By the way, we are already using ruff as linter and formatter!

Understood. From the documentation itself, ruff sees itself as a drop-in replacement for flake8 and other syntactic linters.

Please correct me if I am wrong, but it seems that ruff doesn't check the code for correctness or issues. That is something pylint is doing (but it also doing syntactic linting which you could disable). It did find real bugs in the existing code that are not covered by unit tests.

From my experience, these tools can be useful tools so that you can then focus more on adding interesting features.