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 tests for export job dataset #5160

Merged
merged 13 commits into from
Nov 7, 2022
Merged

Conversation

Marishka17
Copy link
Contributor

@Marishka17 Marishka17 commented Oct 24, 2022

Motivation and context

Added REST API tests to export job datasets & annotations and validate their structure.

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.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 24, 2022

❌ Some checks failed
📄 See logs here

@Marishka17
Copy link
Contributor Author

/check

@github-actions
Copy link
Contributor

github-actions bot commented Oct 25, 2022

❌ Some checks failed
📄 See logs here

@sizov-kirill
Copy link
Contributor

/check

@github-actions
Copy link
Contributor

github-actions bot commented Oct 26, 2022

❌ Some checks failed
📄 See logs here

@Marishka17 Marishka17 changed the title [WIP] Add tests for export job dataset Add tests for export job dataset Nov 3, 2022
os.remove(temp_anno_file_name)

@pytest.mark.parametrize("username, jid", [("admin1", 14)])
def test_check_exported_cvat_dataset_structure(
Copy link
Contributor

@zhiltsov-max zhiltsov-max Nov 4, 2022

Choose a reason for hiding this comment

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

  1. Why only these two formats were tested?
  2. Could you try to avoid copy-paste programming? I suggest to use something similar to what we already have for task and, from recent update, for project export. It can easily be reproduced for jobs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why only these two formats were tested?

I believe that in these tests 2 formats will be enough because tests should check that the annotation file contains only job annotations (the dataset contains only job data) and not the entire task. If this is true for our format, then the same for the others.

def __init__(self, pk):
self.db_job = models.Job.objects.select_related('segment__task') \
.select_for_update().get(id=pk)
def __init__(self, pk, is_prefetched=False):
Copy link
Contributor

@zhiltsov-max zhiltsov-max Nov 4, 2022

Choose a reason for hiding this comment

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

This new argument is unclear to me - when and why it should be used? I don't see how the fact the data is (was?) prefetched affects this class, except it might work slightly more optmizied.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It also affects the sorting of images in the annotation file in cvat format. (We can get sorted sequence from db or sort in code, but I've chosen this approach because we use it for tasks)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how this answers the questions above.

We can get sorted sequence from db or sort in code, but I've chosen this approach because we use it for tasks

I see that sorting is performed in the newly added code when frames are fetched from DB. The DB itself doesn't impose any sorting.

@zhiltsov-max
Copy link
Contributor

/check

@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2022

🚫 Workflows has been canceled
📄 See logs here

@zhiltsov-max zhiltsov-max merged commit ba74709 into develop Nov 7, 2022
@zhiltsov-max zhiltsov-max deleted the mk/tests_export_job_dataset branch November 7, 2022 17:19
@nmanovic nmanovic mentioned this pull request Dec 12, 2022
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.

3 participants