feat: Various fixes and improvements #41

jirimoravcik · 2023-01-30T18:50:35Z

Fixes:

removed tons of todos that I either validated (e.g. vs crawlee) or removed since they're not really relevant
env var for local storage directory now properly handled, also added a test
validation of request queue's request dict via budget ow
LRUCache change yield from to return, narrow return type
memory storage tests: removed write_metadata=True since it's not the default and reworked the tests to work without metadata.

Improvements:

added some more cases in various tests
added an e2e test that simulates 2 local runs. First run uses local storage to create files, second run reads from them. Basically checks if rerunning the actor locally works as intended.

…ueue`

drobnikj · 2023-01-31T10:39:43Z

src/apify/_utils.py

+    ...
+
+
+def _budget_ow(


I guess, there should be some dynamic type checker in python. But I like the name maybe you should create package of it 😄

fnesveda

Cool! Just a few small comments 🙂

fnesveda · 2023-01-31T15:00:04Z

src/apify/_utils.py

+
+
+@overload
+def _budget_ow(value: Union[str, int, float, bool], predicate: Tuple[Type, bool], value_name: str) -> None:  # noqa: U100


😄 😄 great name

fnesveda · 2023-01-31T15:04:55Z

src/apify/_utils.py

+    def validate_single(field_value: Any, expected_type: Type, required: bool, name: str) -> None:
+        if field_value is None and required:
+            raise ValueError(f'"{name}" is required!')
+        actual_type = type(field_value)


I think this is too strict, it won't work for subclasses for example. I'd rather use isinstance here.

fnesveda · 2023-01-31T15:08:06Z

src/apify/memory_storage/memory_storage.py

        self._datasets_directory = os.path.join(self._local_data_directory, 'datasets')
        self._key_value_stores_directory = os.path.join(self._local_data_directory, 'key_value_stores')
        self._request_queues_directory = os.path.join(self._local_data_directory, 'request_queues')
        self._write_metadata = write_metadata if write_metadata is not None else '*' in os.getenv('DEBUG', '')
        self._persist_storage = persist_storage if persist_storage is not None else not any(
-            os.getenv('APIFY_PERSIST_STORAGE', 'true') == s for s in ['false', '0', ''])
+            os.getenv(ApifyEnvVars.PERSIST_STORAGE, 'true') == s for s in ['false', '0', ''])


You could use _maybe_parse_bool here

yeah, code will be nicer

fnesveda · 2023-01-31T15:15:37Z

tests/unit/actor/test_actor_memory_storage_e2e.py

+from apify.storages import StorageManager
+
+
+async def run_e2e_test(monkeypatch: pytest.MonkeyPatch, tmp_path: str, purge_on_start: bool = True) -> None:


You could use pytest.mark.parametrize here:

Suggested change

async def run_e2e_test(monkeypatch: pytest.MonkeyPatch, tmp_path: str, purge_on_start: bool = True) -> None:

@pytest.mark.parametrize('purge_on_start', [True, False])

async def test_actor_memory_storage_e2e(monkeypatch: pytest.MonkeyPatch, tmp_path: str, purge_on_start: bool = True) -> None:

nice, didn't know that decorator

fnesveda

I wanted to click "Request changes", sorry 😄

fnesveda

Cool!

jirimoravcik added 3 commits January 30, 2023 01:22

feat: Add documentation for Dataset, KeyValueStore, and `RequestQ…

37db8ff

…ueue`

Merge branch 'master' into feature/fixes-and-improvements

6860933

feat: Various fixes and improvements

72b8883

github-actions bot assigned jirimoravcik Jan 30, 2023

jirimoravcik changed the title ~~Feature/fixes and improvements~~ feat: Various fixes and improvements Jan 30, 2023

github-actions bot added this to the 56th sprint - Platform team milestone Jan 30, 2023

github-actions bot added the t-platform Issues with this label are in the ownership of the platform team. label Jan 30, 2023

jirimoravcik added the adhoc Ad-hoc unplanned task added during the sprint. label Jan 30, 2023

jirimoravcik requested review from fnesveda and drobnikj and removed request for fnesveda January 30, 2023 18:55

jirimoravcik added 2 commits January 30, 2023 20:03

simplify env var name gettings

8fbfccb

address PR comments

4fb6965

drobnikj approved these changes Jan 31, 2023

View reviewed changes

fnesveda approved these changes Jan 31, 2023

View reviewed changes

fnesveda requested changes Jan 31, 2023

View reviewed changes

jirimoravcik requested a review from fnesveda January 31, 2023 18:17

fnesveda approved these changes Jan 31, 2023

View reviewed changes

jirimoravcik merged commit 5bae238 into master Jan 31, 2023

jirimoravcik deleted the feature/fixes-and-improvements branch January 31, 2023 19:25

fnesveda added the validated Issues that are resolved and their solutions fulfill the acceptance criteria. label Oct 9, 2023

github-actions bot added the tested Temporary label used only programatically for some analytics. label Oct 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Various fixes and improvements #41

feat: Various fixes and improvements #41

jirimoravcik commented Jan 30, 2023 •

edited

Loading

drobnikj Jan 31, 2023

fnesveda left a comment

fnesveda Jan 31, 2023

fnesveda Jan 31, 2023

fnesveda Jan 31, 2023

jirimoravcik Jan 31, 2023

fnesveda Jan 31, 2023

jirimoravcik Jan 31, 2023

fnesveda left a comment

fnesveda left a comment



		@overload
		def _budget_ow(value: Union[str, int, float, bool], predicate: Tuple[Type, bool], value_name: str) -> None: # noqa: U100

		from apify.storages import StorageManager


		async def run_e2e_test(monkeypatch: pytest.MonkeyPatch, tmp_path: str, purge_on_start: bool = True) -> None:

	async def run_e2e_test(monkeypatch: pytest.MonkeyPatch, tmp_path: str, purge_on_start: bool = True) -> None:
	@pytest.mark.parametrize('purge_on_start', [True, False])
	async def test_actor_memory_storage_e2e(monkeypatch: pytest.MonkeyPatch, tmp_path: str, purge_on_start: bool = True) -> None:

feat: Various fixes and improvements #41

feat: Various fixes and improvements #41

Conversation

jirimoravcik commented Jan 30, 2023 • edited Loading

drobnikj Jan 31, 2023

Choose a reason for hiding this comment

fnesveda left a comment

Choose a reason for hiding this comment

fnesveda Jan 31, 2023

Choose a reason for hiding this comment

fnesveda Jan 31, 2023

Choose a reason for hiding this comment

fnesveda Jan 31, 2023

Choose a reason for hiding this comment

jirimoravcik Jan 31, 2023

Choose a reason for hiding this comment

fnesveda Jan 31, 2023

Choose a reason for hiding this comment

jirimoravcik Jan 31, 2023

Choose a reason for hiding this comment

fnesveda left a comment

Choose a reason for hiding this comment

fnesveda left a comment

Choose a reason for hiding this comment

jirimoravcik commented Jan 30, 2023 •

edited

Loading