Enable strict mypy type checking

[kt474]

Summary

I assume the purpose of #88 is to enable show_none_errors which will shows errors related to strict None checking - not sure if this would've caught the issue here. Everything else in mypy is already enabled.

Enabling show_none_errors gives 52 "errors" - I'll have to go through each of them but there will be merge conflicts with the current PRs and I'm not sure all of the errors are worth fixing

Details and comments

Fixes #88

[daka1510]

I assume the purpose of #88 is to enable show_none_errors which will shows errors related to strict None checking - not sure if this would've caught the issue #80 (review). Everything else in mypy is already enabled.

No, the intent of #88 is to ensure that mypy also checks for type errors in our test code. The command we currently use does not run mypy for all files in the test directory.

mypy:
	mypy --module qiskit_ibm_runtime

Bonus points for checking the tutorial code (/docs/tutorials) with mypy as well.

[kt474]

Which specific rules do we want to enable here? When enabled, there are over 400 errors - mostly no-untyped-def, and no-untyped-call

Also, if test_jupyter is not excluded, there are a lot errors in qiskit_ibm_runtime/visualization and qiskit_ibm_runtime/jupyter

[daka1510]

qiskit_ibm_runtime/visualization and qiskit_ibm_runtime/jupyter are dead code paths that we will follow up on via #112, #124. So I agree to exclude it from type checking.

I suggest to start with

[mypy-test.*]
disallow_untyped_calls = False
disallow_untyped_defs = False

and investigate/fix all remaining 60 issues. 41 issues can be fixed very easily by adding an explicit return type (e.g. -> None).

# appears like disallow_untyped_defs does not apply here. If function parameters are partially typed, mypy expects the return type to be explicitly set
test/integration/test_proxies.py:120: error: Function is missing a return type annotation  [no-untyped-def]

[coveralls]

Pull Request Test Coverage Report for Build 1889167529

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.7%) to 57.469%

---

Totals
Change from base Build 1885737893:	0.7%
Covered Lines:	2039
Relevant Lines:	3548

---

💛 - Coveralls

[daka1510]

Thanks for following up, @kt474. Added 2 questions, otherwise looks good.

[daka1510]

Why is this needed?

[kt474]

There are attr-defined errors in the setup and teardown class methods - I can ignore the specific lines instead of ignoring errors for the whole file

[daka1510]

Can we also try to fix it in the test code? Would something like below work instead?

class IBMTestCase(unittest.TestCase):
    """Custom TestCase for use with the Qiskit IBM Runtime."""

    log: logging.Logger
    dependencies: IntegrationTestDependencies
    service: IBMRuntimeService

[daka1510]

That way we can benefit from auto-completion in subclasses that access the class attributes :)

[daka1510]

Does -> FakeAuthClient not work?

[kt474]

Return type "FakeAuthClient" of "_authenticate_legacy_account" incompatible with return type "AuthClient" in supertype "IBMRuntimeService"

There is a mypy override error if FakeAuthClient is used as a return type so we can have Any as a return type or do something like -> 'FakeAuthClient' # type: ignore

[daka1510]

I see. These failures appear to happen because FakeAuthClient is not explicitly set up as a subclass of AuthClient.

How about this:

class FakeRuntimeService(IBMRuntimeService):
   ...
   def _authenticate_legacy_account(
        self, client_params: ClientParameters
    ) -> "FakeAuthClient":
   # ...

class FakeAuthClient(AuthClient):
    """Fake auth client."""

    def __init__(self):  # pylint: disable=super-init-not-called
        # Avoid calling parent __init__ method. It has side-effects that are not supported in unit tests.
        pass

    def current_service_urls(self):
        """Return service urls."""
        return {"http": "legacy_api_url", "services": {"runtime": "legacy_runtime_url"}}

    def current_access_token(self):
        """Return access token."""
        return "some_token"

[daka1510]

Thanks for following up on my proposals, @kt474. Looks good to me. @rathishcholarajan any objections from your side to integrate it?

[rathishcholarajan]

LGTM!