-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Nuclio as a plugin in CVAT, improved system to check installed plugins #2192
Conversation
Why have you decided to hide
|
This solution based on explanation of @nmanovic. |
@zhiltsov-max @nmanovic Hey guys, could you please look at server part of the patch? |
…ore, updated ui version, updated changelog
cvat/apps/plugins/views.py
Outdated
} | ||
if apps.is_installed('cvat.apps.git'): | ||
result['GIT_INTEGRATION'] = True | ||
if os.environ.get("CVAT_ANALYTICS", '0') == '1': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -26,18 +26,18 @@ services: | |||
context: ./components/analytics/kibana | |||
args: | |||
ELK_VERSION: 6.4.0 | |||
depends_on: ['cvat_elasticsearch'] | |||
depends_on: [ 'cvat_elasticsearch' ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason to add extra spaces here?
entrypoint: ['bash', 'wait-for-it.sh', 'elasticsearch:9200', '-t', '0', '--', | ||
'/bin/bash', 'wait-for-it.sh', 'kibana:5601', '-t', '0', '--', | ||
'/usr/bin/python3', 'kibana/setup.py', 'kibana/export.json'] | ||
entrypoint: [ 'bash', 'wait-for-it.sh', 'elasticsearch:9200', '-t', '0', '--', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please keep indentation as is?
@@ -316,7 +319,8 @@ class CVATApplication extends React.PureComponent<CVATAppProps & RouteComponentP | |||
<Route exact path='/tasks/create' component={CreateTaskPageContainer} /> | |||
<Route exact path='/tasks/:id' component={TaskPageContainer} /> | |||
<Route exact path='/tasks/:tid/jobs/:jid' component={AnnotationPageContainer} /> | |||
<Route exact path='/models' component={ModelsPageContainer} /> | |||
{ isModelPluginActive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Boris. It is better to show instructions how to enable the feature in UI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it better to do in separate PR.
event.preventDefault(); | ||
history.push('/models'); | ||
|
||
{isModelsPluginActive && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Show instructions if models plugin isn't available.
Models | ||
</Button> | ||
)} | ||
{isAnalyticsPluginActive && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Show instructions if analytics plugin isn't available.
cvat/apps/plugins/views.py
Outdated
|
||
# Create your views here. | ||
|
||
class Plugins(viewsets.ViewSet): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's implement it not as a separate app but as a part of the engine app (/api/v1/server/plugins). In this case the feature is so tiny feature that I don't think we need a separate django app.
cvat/apps/lambda_manager/urls.py
Outdated
@@ -2,9 +2,10 @@ | |||
# | |||
# SPDX-License-Identifier: MIT | |||
|
|||
from django.urls import include |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import include, path
Let's merge the PR and then add some instructions to activate analytics and models if they are disabled. |
Motivation and context
How has this been tested?
Checklist
develop
branchcvat-core, cvat-data and cvat-ui)
License
Feel free to contact the maintainers if that's a concern.