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

Add tf import checks and tests #1567

Merged
merged 12 commits into from
Jun 4, 2020
Merged

Add tf import checks and tests #1567

merged 12 commits into from
Jun 4, 2020

Conversation

zhiltsov-max
Copy link
Contributor

@zhiltsov-max zhiltsov-max commented May 20, 2020

Closes #1393
Closes #1219
Closes #970

Motivation and context

When tensorflow package was imported, it was causing crash of Python interpreter if there was no AVX support on the machine. This patch adds a check for availability of such import. If a format can not be used, it is shown as inactive.

  • Added filtering for file extension in file selection window when importing annotations.
  • Added tf import availability checks
  • UI now can show existing, but disabled import and export formats

How has this been tested?

Unit tests.

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

@coveralls
Copy link

coveralls commented May 21, 2020

Pull Request Test Coverage Report for Build 5468

  • 37 of 53 (69.81%) changed or added relevant lines in 8 files are covered.
  • 13 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.02%) to 65.692%

Changes Missing Coverage Covered Lines Changed/Added Lines %
datumaro/datumaro/components/project.py 8 9 88.89%
datumaro/datumaro/util/os_util.py 2 3 66.67%
cvat/apps/dataset_manager/formats/tfrecord.py 6 8 75.0%
cvat/apps/engine/views.py 6 9 66.67%
datumaro/datumaro/plugins/openvino_launcher.py 0 3 0.0%
datumaro/datumaro/util/tf_util.py 9 15 60.0%
Files with Coverage Reduction New Missed Lines %
cvat/apps/engine/views.py 1 76.92%
datumaro/datumaro/plugins/openvino_launcher.py 1 3.77%
cvat/apps/engine/media_extractors.py 3 75.29%
src/annotation-formats.js 8 60.0%
Totals Coverage Status
Change from base Build 5455: -0.02%
Covered Lines: 10896
Relevant Lines: 16185

💛 - Coveralls

const pending = (exportActivities || []).includes(exporter);
exporters.map((exporter: any): JSX.Element => {
const pending = (exportActivities || []).includes(exporter.name);
const disabled = !exporter.enabled || pending;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bsekachev, can we show disabled formats in gray?

Copy link
Member

Choose a reason for hiding this comment

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

@zhiltsov-max What means "disabled formats"? Maybe just do not show them at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Existing, but not available.

@zhiltsov-max zhiltsov-max changed the title [WIP] Add tf import checks and tests Add tf import checks and tests May 22, 2020
@zhiltsov-max zhiltsov-max added this to the 1.0.0-release milestone May 22, 2020
@nmanovic nmanovic removed this from the 1.0.0-release milestone May 23, 2020
@zhiltsov-max
Copy link
Contributor Author

@bsekachev, can you review this patch?

@nmanovic
Copy link
Contributor

@zhiltsov-max , I see a couple of conflicts.

@azhavoro azhavoro mentioned this pull request May 27, 2020
@@ -16,8 +16,8 @@ interface Props {
taskMode: string;
bugTracker: string;

loaders: string[];
dumpers: string[];
loaders: any[];
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 add an interface definition for loader and dumper?

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 don't know, can I? I'd just have used the class name, but for some reason they are never used here, so I just followed the style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bsekachev, can we use more specific types here than any for objects?

Copy link
Member

Choose a reason for hiding this comment

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

@zhiltsov-max

On the one hand, of course, we can declare temporary interfaces in cvat-ui for loaders and dumpers and specify them here.
But on the other hand it is only temporary solution. In general case need to rewrite cvat-core on typescript or at least create typings file for the library. This is quite huge change.

className='cvat-menu-dump-submenu-item'
>
<Icon type='download' />
<Text strong={isDefault}>{dumper}</Text>
<Text strong={isDefault} disabled={disabled}>{dumper.name}</Text>
Copy link
Member

Choose a reason for hiding this comment

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

You can try something like that: type={disabled ? 'secondary' : undefined} if you want to do text gray.
But I would suggest do not display such formats. Just using:

dumpers.filter((dumper: any) => dumper.enabled).sort(...

Copy link
Member

Choose a reason for hiding this comment

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

One more option is to use style attribute:

<Text style={disabled ? { opacity: '0.5' } : {}} ...

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 tried to just pass disabled to Icon element, but it didn't work, while it is described in the documentation of the library. I guess, we can be using an outdated version.

Copy link
Member

Choose a reason for hiding this comment

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

@zhiltsov-max

We use version 3.x. It was the latest version when we started doing this UI.
Now version 4.x exists, but migrating is painful

bsekachev
bsekachev previously approved these changes Jun 3, 2020
Copy link
Member

@bsekachev bsekachev left a comment

Choose a reason for hiding this comment

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

Client part is ok. Are you going to do disabled formats "gray"?

@zhiltsov-max
Copy link
Contributor Author

@bsekachev, they actually are. The icons in exports are not, but the text is.

azhavoro
azhavoro previously approved these changes Jun 3, 2020
@zhiltsov-max zhiltsov-max dismissed stale reviews from azhavoro and bsekachev via 8c5528e June 4, 2020 10:18
@bsekachev
Copy link
Member

@zhiltsov-max

Can I merge it?

@zhiltsov-max
Copy link
Contributor Author

@bsekachev, it is ready.

@bsekachev bsekachev merged commit da3fa34 into develop Jun 4, 2020
@nmanovic nmanovic deleted the zm/avx-dependency branch June 6, 2020 04:53
frndmg pushed a commit to signatrix/cvat that referenced this pull request Aug 5, 2020
* Add tf import checks and tests

* implement disabled formats on server

* python 3.5 compatibility

* add checks to dm tests

* fix tests

* Support for disabled formats in UI

* add sorting for formats, mark grey disabled items

* update changelog

* advance package versions
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.

AVX is required to run CVAT (should be optional) Can't export dataset Error during Export of TfRecords
5 participants