Skip to content
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

Merged
merged 8 commits into from Sep 25, 2020

Conversation

dmitryagapov
Copy link
Contributor

@dmitryagapov dmitryagapov commented Sep 17, 2020

Motivation and context

How has this been tested?

Checklist

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.
  • I have updated the license header for each file (see an example below)
# Copyright (C) 2020 Intel Corporation
#
# SPDX-License-Identifier: MIT

cvat-core/src/server-proxy.js Outdated Show resolved Hide resolved
cvat-core/src/server-proxy.js Outdated Show resolved Hide resolved
cvat-core/src/server-proxy.js Outdated Show resolved Hide resolved
cvat-ui/src/actions/meta-action.ts Outdated Show resolved Hide resolved
cvat-core/src/api.js Outdated Show resolved Hide resolved
cvat-ui/src/reducers/meta-reducer.ts Outdated Show resolved Hide resolved
cvat-ui/src/components/header/header.tsx Outdated Show resolved Hide resolved
@bsekachev
Copy link
Member

bsekachev commented Sep 18, 2020

Why have you decided to hide Models button when optionally?

  1. We are still able to go manually to /models
  2. Early CVAT showed instruction to deploy a new model if no models available. So, user understood that CVAT is able to work with models.
  3. There are other components (like button Automatic Annotation (and probably others)) which we should hide in this case.

@dmitryagapov
Copy link
Contributor Author

This solution based on explanation of @nmanovic.

@bsekachev bsekachev changed the title allow to run cvat without nuclio Nuclio as a plugin in CVAT, improved system to check installed plugins Sep 21, 2020
bsekachev
bsekachev previously approved these changes Sep 21, 2020
@bsekachev
Copy link
Member

@zhiltsov-max @nmanovic Hey guys, could you please look at server part of the patch?

@coveralls
Copy link

coveralls commented Sep 21, 2020

Coverage Status

Coverage decreased (-0.04%) to 65.311% when pulling 842b34e on da/allow_to_run_cvat_without_nuclio into a93b460 on develop.

}
if apps.is_installed('cvat.apps.git'):
result['GIT_INTEGRATION'] = True
if os.environ.get("CVAT_ANALYTICS", '0') == '1':
Copy link
Contributor

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' ]
Copy link
Contributor

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', '--',
Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor Author

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 && (
Copy link
Contributor

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 && (
Copy link
Contributor

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.


# Create your views here.

class Plugins(viewsets.ViewSet):
Copy link
Contributor

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.

@@ -2,9 +2,10 @@
#
# SPDX-License-Identifier: MIT

from django.urls import include
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import include, path

@bsekachev
Copy link
Member

Let's merge the PR and then add some instructions to activate analytics and models if they are disabled.
I will create an issue in advance and assign it to @dmitryagapov

@bsekachev bsekachev merged commit f2c84a2 into develop Sep 25, 2020
@bsekachev bsekachev deleted the da/allow_to_run_cvat_without_nuclio branch September 25, 2020 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants