Use absolute imports #161

honzajavorek · 2024-01-03T21:13:10Z

Implements #160, with exception of tests, where the relative imports make some sense and removing them would mean re-architecturing the tests.

I've ran make check-code and it did complain about Import block is un-sorted or un-formatted after my changes, so I've ran it with --fix and committed the changes, but then I've hit the following error caused by circular import:

Lint codebase.................................................................Passed
Type-check codebase...........................................................Passed
Run unit tests................................................................Failed
- hook id: unit-tests
- exit code: 2

python3 -m pytest -n auto -ra tests/unit --cov=src/apify
ImportError while loading conftest '/Users/honza/Projects/apify-sdk-python/tests/unit/conftest.py'.
tests/unit/conftest.py:11: in <module>
    from apify import Actor
src/apify/__init__.py:3: in <module>
    from apify.actor import Actor
src/apify/actor.py:27: in <module>
    from apify.storages import Dataset, KeyValueStore, RequestQueue, StorageClientManager
src/apify/storages/__init__.py:1: in <module>
    from apify.storages.dataset import Dataset
src/apify/storages/dataset.py:11: in <module>
    from apify.storages.key_value_store import KeyValueStore
src/apify/storages/key_value_store.py:7: in <module>
    from apify_client.clients import KeyValueStoreClientAsync, KeyValueStoreCollectionClientAsync
venv/lib/python3.11/site-packages/apify_client/__init__.py:3: in <module>
    from .client import ApifyClient, ApifyClientAsync
venv/lib/python3.11/site-packages/apify_client/client.py:6: in <module>
    from .clients import (
venv/lib/python3.11/site-packages/apify_client/clients/__init__.py:1: in <module>
    from .base import (
venv/lib/python3.11/site-packages/apify_client/clients/base/__init__.py:1: in <module>
    from .actor_job_base_client import ActorJobBaseClient, ActorJobBaseClientAsync
venv/lib/python3.11/site-packages/apify_client/clients/base/actor_job_base_client.py:11: in <module>
    from apify._errors import ApifyApiError
E   ModuleNotFoundError: No module named 'apify._errors'

I think the circular import wasn't there, but sorting the imports as required by ruff probably introduced it. I did not investigate further, as this was supposed to be just a small gift, not a all evening debugging session 😅

I intentionally committed both changes separately, so that you can cherry pick what you like and drop the rest.

Implements apify#160, with exception of tests, where the relative imports make some sense and removing them would mean re-architecturing the tests.

vdusek · 2024-01-04T11:07:10Z

Thank you @honzajavorek for your contribution 🙂, I'll try to investigate the problem with circular imports.

honzajavorek · 2024-01-04T12:03:10Z

The ruff fix in c362145 moves the apify_shared imports down below local apify imports as it's trying to sort them alphabetically, which causes the circular imports.

I suppose the problem is caused by ruff then. I don't use ruff, so I'm unsure whether it has some settings to sort imports in a different way. I can recommend isort which is able to correctly detect which imports are from the local package and then make ~three groups (stdlib, pypi packages, local modules).

vdusek · 2024-01-05T08:58:48Z

Nice, thank you for the investigation @honzajavorek.

We recently switched completely to Ruff for linting and formatting. As far as I know, the Isort functionality should be integrated into Ruff, but there might be some inconsistencies.

However, the reason for this sorting is the configuration in pyproject.toml:

[tool.ruff.lint.isort]
known-first-party = ["apify", "apify_client", "apify_shared"]

To solve this issue, we could update it like this:

[tool.ruff.lint.isort]
known-first-party = ["apify_client", "apify_shared"]
known-local-folder = ["apify"]

@janbuchar, do you have any thoughts on this?

janbuchar · 2024-01-05T09:38:56Z

Updating the known first party option like you suggest seems pretty clean to me.

Or we could keep imports from the same level relative (.foo is fine, ..foo is not), but that still may break in some cases.

vdusek · 2024-01-05T13:58:07Z

So let's update the configuration as suggested above. It should keep the order of the imports unchanged.

honzajavorek · 2024-01-05T15:37:54Z

I had the project open so I made the change. It fails on similar issue in tests, albeit I think I didn't make any or many changes in tests.

ImportError while loading conftest '/Users/honza/Projects/apify-sdk-python/tests/unit/conftest.py'.
tests/unit/conftest.py:11: in <module>
    from apify_client.client import ApifyClientAsync
venv/lib/python3.11/site-packages/apify_client/__init__.py:3: in <module>
    from .client import ApifyClient, ApifyClientAsync
venv/lib/python3.11/site-packages/apify_client/client.py:6: in <module>
    from .clients import (
venv/lib/python3.11/site-packages/apify_client/clients/__init__.py:1: in <module>
    from .base import (
venv/lib/python3.11/site-packages/apify_client/clients/base/__init__.py:1: in <module>
    from .actor_job_base_client import ActorJobBaseClient, ActorJobBaseClientAsync
venv/lib/python3.11/site-packages/apify_client/clients/base/actor_job_base_client.py:11: in <module>
    from apify._errors import ApifyApiError
src/apify/__init__.py:3: in <module>
    from apify.actor import Actor
src/apify/actor.py:11: in <module>
    from apify_client import ApifyClientAsync
E   ImportError: cannot import name 'ApifyClientAsync' from partially initialized module 'apify_client' (most likely due to a circular import) (/Users/honza/Projects/apify-sdk-python/venv/lib/python3.11/site-packages/apify_client/__init__.py)

I think I should have nuke the commit with ruff fix and try to apply the pyproject.toml changes directly to the initial commit of this PR, perhaps the result would be better, but that is a task for another day. Also I think this wouldn't be hard to pin point manually and fix, but I can't attend to it right now.

vdusek

@honzajavorek Thank you for your contribution! I got only little things 🙂.

Circular import in unit tests

I think there is currently no circular import in the unit tests - the CI passed and when I executed them locally it worked as well, could you please try it again?

Update version of Client & Shared

I already introduced these changes in our other Python packages, that are used by the SDK (Relative imports are replaced with absolute imports apify-client-python#176, Relative imports are replaced for absolute imports apify-shared-python#19) - could you please bump the version of both of them in the pyproject.toml to the latest one?

Sorting configuration

Please update the sorting configuration in the pyproject.py to just the following:

[tool.ruff.lint.isort]
known-local-folder = ["apify"]

make sure to execute make format after changing this 🙂.

CHANGELOG

Could you please describe the change in the CHANGELOG.md? See the https://github.com/apify/apify-client-python/blob/v1.6.2/CHANGELOG.md?plain=1#L5:L7 for inspiration.

Stop ignoring `TID252` violation

Please remove the rule TID252 from the list of ignored violations in the pyproject.toml -> tool.ruff -> ignore

Undo changes in all `init.py`

Could you please undo all the changes in __init__.py files? They contain only imports from their siblings and it does not make much sense to use absolute imports there.

Integration tests

They're failing in the CI because the PR is from the fork, I executed them locally and they're fine as well 💪.

vdusek · 2024-01-08T15:29:22Z

src/apify/_memory_storage/__init__.py

@@ -1,3 +1,3 @@
-from .memory_storage_client import MemoryStorageClient
+from apify._memory_storage.memory_storage_client import MemoryStorageClient


Please, let's leave all the __init__.py with relative imports, since they're importing only from their siblings.

Suggested change

from apify._memory_storage.memory_storage_client import MemoryStorageClient

from .memory_storage_client import MemoryStorageClient

vdusek · 2024-01-08T15:32:12Z

pyproject.toml

@@ -147,7 +147,8 @@ docstring-quotes = "double"
 inline-quotes = "single"

 [tool.ruff.lint.isort]
-known-first-party = ["apify", "apify_client", "apify_shared"]
+known-first-party = ["apify_client", "apify_shared"]


Could you please remove known-first-party completely and leave there only known-local-folder = ["apify"]?

Suggested change

known-first-party = ["apify_client", "apify_shared"]

known-local-folder = ["apify"]

We agreed with @janbuchar to not to create a new section in our imports, so apify-shared and apify-client will be part of standard 3rd party imports.

@vdusek

With our [first community PR](#161) (🎉 ), there was an issue that the integration tests for the PR did not run since they could not access the API token for the test user from the repo secrets. This changes the check workflows so that the tests run on the `pull_request_target` event, not on the `pull_request` event, which allows passing the secrets to the workflow even in case of forks. To prevent leaking the secrets, running the workflows on a PR from a fork requires approving the workflow run by a maintainer via an environment protection rule. I've put @vdusek, @janbuchar, @jirimoravcik and me as maintainers of that environment, so that we get notified in case of a fork PR and can approve the workflow runs. The `pull_request_target` event runs in the context of the PR base branch, not the head branch, for security reasons, so to run the tests against the PR code, we have to check it out explicitly instead of checking out the default ref.

honzajavorek · 2024-01-09T10:06:17Z

Thanks for the review! That's a lot of extra tasks! 😅 I'll get back to this as I'm learning new things on the way (already started using ruff on one of my projects!). But later, now there are some work duties and then skiing ⛷️

honzajavorek · 2024-01-09T10:08:07Z

I see in 27b752a that you are learning about new things too 😄

fnesveda · 2024-01-09T10:11:56Z

I see in 27b752a that you are learning about new things too 😄

Yeah, that was an adventure 😄 Maybe you could try merging current master to your PR branch, to see if it works as expected?

vdusek · 2024-01-09T17:39:57Z

Thanks for the review! That's a lot of extra tasks! 😅

@honzajavorek Don't worry, they should be easy to solve. Sure, enjoy your skiing 🙂.

vdusek · 2024-01-19T08:30:48Z

@honzajavorek Based on our Discord conversation ... I will close this and open a new PR, where I'll use your (squashed) commits, solve the minor issues mentioned in #161 (review), and merge it. Thank you.

PR here - #177.

honzajavorek · 2024-01-19T10:08:39Z

Yup, it would take me too long to get back to this as it's low priority for me right now. Thanks for all the work!

honzajavorek added 2 commits January 3, 2024 22:00

Use absolute imports

c44b672

Implements apify#160, with exception of tests, where the relative imports make some sense and removing them would mean re-architecturing the tests.

fix formatting

c362145

vdusek self-requested a review January 4, 2024 10:44

prevent ruff to re-order imports

b6e4ecf

fnesveda mentioned this pull request Jan 8, 2024

Add option to run integration tests from PRs from forks #167

Merged

vdusek requested changes Jan 8, 2024

View reviewed changes

vdusek mentioned this pull request Jan 19, 2024

Use absolute imports #177

Merged

vdusek closed this Jan 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use absolute imports #161

Use absolute imports #161

honzajavorek commented Jan 3, 2024 •

edited

vdusek commented Jan 4, 2024

honzajavorek commented Jan 4, 2024 •

edited

vdusek commented Jan 5, 2024 •

edited

janbuchar commented Jan 5, 2024

vdusek commented Jan 5, 2024

honzajavorek commented Jan 5, 2024

vdusek left a comment •

edited

vdusek Jan 8, 2024

vdusek Jan 8, 2024

honzajavorek commented Jan 9, 2024

honzajavorek commented Jan 9, 2024

fnesveda commented Jan 9, 2024

vdusek commented Jan 9, 2024

vdusek commented Jan 19, 2024

honzajavorek commented Jan 19, 2024

		@@ -1,3 +1,3 @@
		from .memory_storage_client import MemoryStorageClient
		from apify._memory_storage.memory_storage_client import MemoryStorageClient

	known-first-party = ["apify_client", "apify_shared"]
	known-local-folder = ["apify"]

Use absolute imports #161

Use absolute imports #161

Conversation

honzajavorek commented Jan 3, 2024 • edited

vdusek commented Jan 4, 2024

honzajavorek commented Jan 4, 2024 • edited

vdusek commented Jan 5, 2024 • edited

janbuchar commented Jan 5, 2024

vdusek commented Jan 5, 2024

honzajavorek commented Jan 5, 2024

vdusek left a comment • edited

Choose a reason for hiding this comment

Circular import in unit tests

Update version of Client & Shared

Sorting configuration

CHANGELOG

Stop ignoring TID252 violation

Undo changes in all __init__.py

Integration tests

vdusek Jan 8, 2024

Choose a reason for hiding this comment

vdusek Jan 8, 2024

Choose a reason for hiding this comment

honzajavorek commented Jan 9, 2024

honzajavorek commented Jan 9, 2024

fnesveda commented Jan 9, 2024

vdusek commented Jan 9, 2024

vdusek commented Jan 19, 2024

honzajavorek commented Jan 19, 2024

honzajavorek commented Jan 3, 2024 •

edited

honzajavorek commented Jan 4, 2024 •

edited

vdusek commented Jan 5, 2024 •

edited

vdusek left a comment •

edited

Stop ignoring `TID252` violation

Undo changes in all `init.py`