Feature/logger and redis and api improvements #16

oleksii-symon-corva-ai · 2021-01-15T13:51:01Z

improved cache type hinting
improved cache tests
improved api type hinting
deleted logger
deleted obsolete fixtures

…ion tutorial

… feature/DC-754_migrate-scheduled-app-into-middleware # Conflicts: # corva/application.py # corva/scheduled.py # tests/app/test_scheduled.py # tests/conftest.py

This reverts commit 4d80ba1

…mprovements' into feature/minor-logger-and-redis-improvements

…nto feature/minor-logger-and-redis-improvements

finomayato

Before making improvements I'd like to try out existing implementation. I think we are ready to work with bare requests and bare Redis adapter to build an application

finomayato · 2021-01-15T15:45:45Z

corva/network/api.py

+    @property
+    def get(self):
+        return self._request('GET')


why are all methods are properties?

They were made properties to get proper parameter information:

Before:

After:

it does not sound like a good idea to use language features to force IDE to behave in the way you want it to.
Since get is a method of the Api class - it's ok to have it as the first parameter in parameters

finomayato · 2021-01-15T15:46:23Z

corva/network/api.py

+    def _request(self, method: str):
+        def _request_helper(
+             path: str,
+             *,
+             data: Optional[dict] = None,  # request body
+             params: Optional[dict] = None,  # url query string params
+             headers: Optional[dict] = None,  # additional headers to include in request
+             max_retries: Optional[int] = None,  # custom value for max number of retries
+             timeout: Optional[int] = None,  # request timeout in seconds
+        ) -> Response:
+            if method not in self.ALLOWED_METHODS:


Overcomplicated. We can make do without a nested function

I didn't find another way to get proper parameter information like displayed above

make user-facing function accept these as parameters?

finomayato · 2021-01-15T15:47:53Z

corva/state/redis_adapter.py

-        name = name or self.default_name
-
-        n_set = super().hset(name=name, key=key, value=value, mapping=mapping)
+        n_set = super().hset(name=self.name, key=key, value=value, mapping=mapping)

-        if expiry is None and self.pttl(name=name) > 0:
-            self.persist(name=name)
+        if expiry is None and self.pttl() > 0:
+            self.persist(name=self.name)

        if expiry is not None:
-            self.expire(name=name, time=expiry)
+            self.expire(name=self.name, time=expiry)


need documentation on why do we re-define hset default behavior

finomayato · 2021-01-15T15:48:43Z

corva/state/redis_adapter.py

+    def delete(self) -> int:
+        return super().delete(self.name)


Is there any way we can use a non-modified Redis adapter?

yes, we can. logic from overridden functions in RedisAdapter can be moved to corresponding functions in RedisState (e.g move logic from RedisAdapter.hset to RedisState.store). we will get rid of RedisAdapter class and logic will become more straightforward.

jordanambra

I have some general questions here, as well as inline feedback on the PR:

What task does this correspond to?
Why are we refactoring the API interface?
Why are we removing the logger?

There are a lot of different changes all wrapped up into a single PR here. It would be nice to limit this to only API, only Redis, or only logger changes, if those are necessary. Can we justify this PR?

jordanambra · 2021-01-16T03:33:34Z

corva/app/base.py

@@ -52,15 +47,13 @@ def _run(self, event: Event) -> None:
        try:
            context = self.get_context(event=event)
        except Exception:


These try/except blocks seem pointless now that we are just reraising.

jordanambra · 2021-01-16T03:35:50Z

corva/network/api.py

@@ -54,20 +54,25 @@ def _init_session(api_key: str, app_name: str, max_retries: int, allowed_methods

        return session

-    def get(self, path: str, **kwargs):
-        return self._request('GET', path, **kwargs)
+    @property


You should leave these as functions and call or pass parameters through directly instead of using a layer of indirection with properties and wrapped functions. See the requests library itself for an example, or just revert back to how it was.

The properties were added with the purpose of getting proper parameter information in IDE. Do you think it'd be better to use functions like before, but without hints?

#16 (comment)

oleksii-symon-corva-ai · 2021-01-18T09:42:36Z

@finomayato

I think we are ready to work with bare requests

We can get rid of Api class and instead pass user a Session instance with binded Authorization headers. In this case users will have to write all timeouts and retries themselves. I think it will simplify the code and make the library more intuitive, what do you think?

oleksii-symon-corva-ai · 2021-01-18T10:37:57Z

@jordanambra

What task does this correspond to?

It doesnt directly correspond to any task but DC-741. These are small changes, that I've spent like an hour on, but thought would be useful enough to compensate for that time.

Why are we refactoring the API interface?

Api and Redis interfaces were refactored to add parameter information displayed here

Why are we removing the logger?

Users should define their own loggers and the library doesnt use it also

There are a lot of different changes ...

Should probably split the PR then and create corresponding tasks in JIRA

finomayato · 2021-01-18T14:26:57Z

We can get rid of Api class and instead pass user a Session instance with binded Authorization headers. In this case users will have to write all timeouts and retries themselves. I think it will simplify the code and make the library more intuitive, what do you think?

Possibly. But not for certain. We will know for sure when we implement the first app with this library. For now, we can just expose AUTH_HEADERS. Or create a really lightweight class Api with methods: get and post which basically has the next implementation:

class Api:
   def get(self, url, **kwargs):
        headers = kwargs.pop('headers', None)
        if headers:
             headers['Token'] = f'Bearer {AUTH_TOKEN}'
        return requests.get(url, headers=headers, **kwargs)

The one and only important question is: how a user will interact with this library?

jordanambra · 2021-01-18T14:41:26Z

Can we leave this PR on hold for now? I don't want to make significant interface changes on this, and I'd prefer that we focus just on the stream/scheduled app implementation until that's released.

oleksii-symon-corva-ai · 2021-01-18T15:28:31Z

Yes, lets hold

oleksii-symon-corva-ai added 30 commits December 23, 2020 13:54

added ui examples

10d2b00

moved config params from Config class to Corva.__init__ in configurat…

e61af57

…ion tutorial

use user_job(event) instead of user_job.run(event)

2658390

deleted unused files and dir

7c682cf

added proper BaseEvent class

df921d4

Added more parameters to BaseContext

5c2a197

Added UserCallableMiddleware

a3dbca2

added new parameters to BaseContext

88625ea

added Middleware class

c84d47f

added UserMiddleware class

27c6a22

fix duplicate path import

40182c5

added __init__.py to corva/middleware/

ee2feba

added Corva class

1014a73

deleted return type annotation from Corva.__init__

71b8838

fixed spelling error

66059f5

added return to UserMiddleware.__call__

50a4460

fixed import order

b27015a

fixed parameter ordering in UserCallableMiddleware.__init__

7f3820f

fixed circular imports

72e48cc

skipped not needed failing tests

0dcc056

fix lint

75ef5ed

added loader middleware

1ea5f39

reworked BaseLoader to be generic

190c71b

updated StreamLoader

4f760e1

reworked BaseLoader

eecbaf4

reworked StreamLoader

a6995e2

fixed return in StreamLoader.load

b442de1

reworked ScheduledLoader

deacb2d

reworked TaskLoader

8826713

either skipped or deleted not needed tests

606bf08

oleksii-symon-corva-ai added 17 commits January 15, 2021 12:35

simplified BaseContext.cache

ebb23ed

simplified BaseContext.store_cache_data

163468e

fixed imports

e235c63

set StreamStateData default values to None

4ef9755

added comment for FakeServer in conftest.patch_redis_adapter

9a73a9d

added comments to fixtures in conftest.py

5750891

deleted app fixture from conftest.py

9113583

Merge branch 'feature/DC-753_migrate-stream-app-into-middleware' into…

7c05707

… feature/DC-754_migrate-scheduled-app-into-middleware # Conflicts: # corva/application.py # corva/scheduled.py # tests/app/test_scheduled.py # tests/conftest.py

merge fixes

682afae

deleted logger.py

4d80ba1

Revert "deleted logger.py"

ff547ac

This reverts commit 4d80ba1

deleted logger.py

278dab2

refactored redis state with its tests

2376003

improved Api type hints

ffbdd88

improved Api type hints

a1abf87

deleted api and corva_settings fixtures from conftest.py

49418a2

Merge remote-tracking branch 'origin/feature/minor-logger-and-redis-i…

39c3cfb

…mprovements' into feature/minor-logger-and-redis-improvements

oleksii-symon-corva-ai requested a review from finomayato January 15, 2021 13:52

oleksii-symon-corva-ai changed the title ~~Feature/minor logger and redis improvements~~ Feature/logger and redis and api improvements Jan 15, 2021

oleksii-symon-corva-ai added 2 commits January 15, 2021 17:34

moved corva/scheduled.py and corva/stream.py to corva/runners package

e700046

Merge branch 'feature/DC-754_migrate-scheduled-app-into-middleware' i…

9042509

…nto feature/minor-logger-and-redis-improvements

finomayato suggested changes Jan 15, 2021

View reviewed changes

jordanambra requested changes Jan 16, 2021

View reviewed changes

Base automatically changed from feature/DC-754_migrate-scheduled-app-into-middleware to master January 20, 2021 15:38

oleksii-symon-corva-ai added a commit that referenced this pull request May 20, 2022

Mypy fix #16

edfd8a5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/logger and redis and api improvements #16

Feature/logger and redis and api improvements #16

oleksii-symon-corva-ai commented Jan 15, 2021 •

edited

Loading

finomayato left a comment •

edited

Loading

finomayato Jan 15, 2021

oleksii-symon-corva-ai Jan 18, 2021

finomayato Jan 18, 2021

finomayato Jan 15, 2021

oleksii-symon-corva-ai Jan 18, 2021

finomayato Jan 18, 2021

finomayato Jan 15, 2021

finomayato Jan 15, 2021

oleksii-symon-corva-ai Jan 18, 2021

jordanambra left a comment

jordanambra Jan 16, 2021

jordanambra Jan 16, 2021

oleksii-symon-corva-ai Jan 18, 2021 •

edited

Loading

oleksii-symon-corva-ai commented Jan 18, 2021

oleksii-symon-corva-ai commented Jan 18, 2021

finomayato commented Jan 18, 2021

jordanambra commented Jan 18, 2021

oleksii-symon-corva-ai commented Jan 18, 2021

Feature/logger and redis and api improvements #16

Are you sure you want to change the base?

Feature/logger and redis and api improvements #16

Conversation

oleksii-symon-corva-ai commented Jan 15, 2021 • edited Loading

finomayato left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanambra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleksii-symon-corva-ai Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

oleksii-symon-corva-ai commented Jan 18, 2021

oleksii-symon-corva-ai commented Jan 18, 2021

finomayato commented Jan 18, 2021

jordanambra commented Jan 18, 2021

oleksii-symon-corva-ai commented Jan 18, 2021

oleksii-symon-corva-ai commented Jan 15, 2021 •

edited

Loading

finomayato left a comment •

edited

Loading

oleksii-symon-corva-ai Jan 18, 2021 •

edited

Loading