modification for reaxflow-workflow-service #70
Conversation
|
Why is this assigned to me? Should I do something, or just review it? |
|
It was to review. |
|
since this is dependent on materials-marketplace/standard-app-api#56, it cannot be reviewed until that one is done |
| @@ -8,6 +8,8 @@ | |||
| from .base import _MarketPlaceAppBase | |||
| from .utils import _decode_metadata, _encode_metadata | |||
|
|
|||
| DEFAULT_COLLECTION_NAME = "DEFAULT_COLLECTION" | |||
There was a problem hiding this comment.
Why isn't the default defined at the Standard app API level?
There was a problem hiding this comment.
I thought it would be easy to integrate if in case we need a name change for default collection. SInce standardapp requires rebuilding production. But anyway now I think it soesnot make sense to change the name once datasink application is established. I can move it to standard app if needed.
marketplace/app/v0/system.py
Outdated
| from .base import _MarketPlaceAppBase | ||
|
|
||
|
|
||
| class MarketPlaceSystemApp(_MarketPlaceAppBase): |
There was a problem hiding this comment.
Why aren't these capabilities directly defined in _MarketPlaceAppBase, loke globalSearch or heartbeat?
There was a problem hiding this comment.
Yeah makes sense. Since there was an additional system level file in standardapp, I added a new one here. I will move them to base.
| self, new_transformation: transformation.NewTransformationModel | ||
| self, | ||
| new_transformation: transformation.NewTransformationModel, | ||
| config: dict = None, |
There was a problem hiding this comment.
The purpose of config is unclear to me, when we already have params. This should be better documented (not inline, but a proper docstring)
There was a problem hiding this comment.
To send additional query parameters which are not defined in Marketplace. For example reaxpro-service needs model_name an additional query parameter to create the transformation. Any key values that are passed in config will be send as query parameters to the application by marketplace. An alternative solution can be to send inside the body. I thought sending it as query will make it less restriction at the application side.
There was a problem hiding this comment.
I still think needs a proper docstring (so users can see it through contextual help in their IDEs) would be specially benefitial in this instance.
.env_template
Outdated
| @@ -8,7 +8,9 @@ MP_ACCESS_TOKEN=.... | |||
| CLIENT_ID="2c791805-ea52-4446-af97-80c0355a73b4" | |||
There was a problem hiding this comment.
This client_id should not be fixed
There was a problem hiding this comment.
Also, it is not really a client_id, is it? It's the app id. Otherwise, what is the difference to KEYCLOAK_CLIENT_ID?
There was a problem hiding this comment.
Keycloak client_id and client_id are different. As per my understanding inorder to validate an user with username and password using keycloak package, we have to use keycloak_client_id "frontend" which is different from the application id.
| from requests import Response | ||
|
|
||
| from .version import __version__ | ||
|
|
||
| MP_DEFAULT_HOST = "https://materials-marketplace.eu/" | ||
|
|
||
|
|
||
| def configure_token(func): |
There was a problem hiding this comment.
All these methods should have proper docstrings that can help users, rather than inline comments
marketplace/client.py
Outdated
| except Exception: | ||
| return None |
There was a problem hiding this comment.
This means all errors will return None, which in your configure_token will trigger an authentication error, right?
- Are you sure those are all the possible errors? What is keycloak is down, for example?
- I would handle exceptions properly and propragate when necessary, rather than catching -> convert to None -> raise new exception
There was a problem hiding this comment.
Now I catch all exceptions and display a generic message "Authentication failure" along with actual reason for the error. Any error other than authentication will be caught separately and displayed to user.
marketplace/client.py
Outdated
| @@ -55,20 +100,26 @@ def _request(self, op, path, **kwargs) -> Response: | |||
| full_url = urljoin(self.marketplace_host_url, path) | |||
| return op(url=full_url, **kwargs) | |||
|
|
|||
| @configure_token | |||
There was a problem hiding this comment.
Is this needed for all the methods, or would it suffice to do it at the _request level?
There was a problem hiding this comment.
Makes sense. I have changed it in latest code.
No description provided.