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

Git formats flexibility #3708

Merged
merged 1 commit into from
Oct 22, 2021
Merged

Conversation

PMazarovich
Copy link
Contributor

@PMazarovich PMazarovich commented Sep 22, 2021

Motivation and context

Sometimes it is necessary to dump and push annotations data to git not only in "CVAT for video" format. With these changes it is possible to choose, which format to use for dumping annotations to git.

How has this been tested?

It was tested manually

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) 2021 Intel Corporation
#
# SPDX-License-Identifier: MIT

format_name = "CVAT for images 1.1"
elif format not in EXPORT_FORMATS:
format_name = "CVAT for video 1.1"
return format_name
Copy link
Contributor

@zhiltsov-max zhiltsov-max Sep 22, 2021

Choose a reason for hiding this comment

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

Could you explain the logic behind this function? If it is only supposed for returning an initial value, maybe it's better to be renamed and/or moved to another place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an additional check if we can use selected git format with this task mode and if this format is presented in EXPORT_FORMATS. Also you may see this logic here: cvat/apps/dataset_repo/dataset_repo.py line 268 (in this branch moved to registry.py)

@ActiveChooN
Copy link
Contributor

@PMazarovich, could you please fix tests and linter problems?

@PMazarovich PMazarovich force-pushed the git_format_flexibility branch 4 times, most recently from aab7055 to 6427625 Compare September 30, 2021 15:16
@PMazarovich
Copy link
Contributor Author

@ActiveChooN, linter fixed (except bandit, not sure, what to fix there), test changed accordingly

@@ -276,15 +280,37 @@ class AdvancedConfigurationForm extends React.PureComponent<Props> {
);
}

private renderGitFormat(): JSX.Element {
const { dumpers } = getCVATStore().getState().formats.annotationFormats.dumpers;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bad practice to use state directly, better to put dumpers in mapStateToProps function. Also I have an error have due to dumpers always is undefined.

@ActiveChooN
Copy link
Contributor

@PMazarovich, hi, do you think you could fix problems in this PR?

@PMazarovich
Copy link
Contributor Author

PMazarovich commented Oct 12, 2021

@ActiveChooN , hello, I'll try to fix them this week

@PMazarovich PMazarovich force-pushed the git_format_flexibility branch 2 times, most recently from 6747ac9 to f47be81 Compare October 14, 2021 17:18
@PMazarovich PMazarovich force-pushed the git_format_flexibility branch from f47be81 to 52353f2 Compare October 15, 2021 08:49
@PMazarovich
Copy link
Contributor Author

@ActiveChooN , hello, requested changes done.

Copy link
Contributor

@ActiveChooN ActiveChooN left a comment

Choose a reason for hiding this comment

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

@PMazarovich, everything works like a charm, thank you for your contribution!
@dvkruchinin, there is a Bandit error not related to that PR, can you help, please? @bsekachev, I can see you were the original contributor of error-related changes in _have_no_access_exception.

@dvkruchinin
Copy link
Contributor

@ActiveChooN

there is a Bandit error not related to that PR, can you help, please?
Bandit fully checks the file included in commit. Perhaps this error can be corrected later.

@nmanovic nmanovic merged commit fb6305f into cvat-ai:develop Oct 22, 2021
@nmanovic
Copy link
Contributor

@PMazarovich , thanks for your contribution! Great job.

@PMazarovich PMazarovich deleted the git_format_flexibility branch October 26, 2021 08:53
@azhavoro azhavoro mentioned this pull request Oct 26, 2021
4 tasks
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.

5 participants