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

[GSoC2024] added filter to not include gt_job when the test intends to create one. #7623

Merged
merged 17 commits into from
Mar 21, 2024

Conversation

Viditagarwal7479
Copy link
Contributor

Fixes #7622

Added a check to ensure that tests which need to create a gt_job aren't picking up tasks which already have a gt_job

How has this been tested?

I added a new task to the testing database and ran these updated tests.

Checklist

  • I submit my changes into the develop branch
    - [ ] I have created a changelog fragment
    - [ ] I have updated the documentation accordingly
  • I have added tests to cover my changes
  • I have linked related issues (see GitHub docs)
    - [ ] I have increased versions of npm packages if it is necessary
    (cvat-canvas,
    cvat-core,
    cvat-data and
    cvat-ui)

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.

Copy link

codecov bot commented Mar 17, 2024

Codecov Report

Merging #7623 (de646ad) into develop (d1a300f) will increase coverage by 0.00%.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #7623   +/-   ##
========================================
  Coverage    83.44%   83.45%           
========================================
  Files          373      373           
  Lines        39723    39723           
  Branches      3741     3741           
========================================
+ Hits         33147    33149    +2     
+ Misses        6576     6574    -2     
Components Coverage Δ
cvat-ui 79.25% <ø> (+<0.01%) ⬆️
cvat-server 87.32% <ø> (+<0.01%) ⬆️

@Viditagarwal7479 Viditagarwal7479 changed the title added filter to not include gt_job when the test intends to create one. [GSoC2024] added filter to not include gt_job when the test intends to create one. Mar 17, 2024
Copy link
Contributor

@zhiltsov-max zhiltsov-max left a comment

Choose a reason for hiding this comment

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

Please resolve the comment, and it can be merged.

@Viditagarwal7479
Copy link
Contributor Author

Viditagarwal7479 commented Mar 18, 2024

The jobs.json and tasks.json files include information about admin1 only (because). In the above test, it's making API requests as admin2 because of which there is a mismatch in validating whether gt_job exists for that task_id.
here it's checking presence of gt_job through the json files
here it's checking presence of gt_job through API request

@Viditagarwal7479
Copy link
Contributor Author

Probably, there's some other issue,
Only entry in jobs.json for user: admin1 and task_id:5 :

    {
      "assignee": {
        "first_name": "Worker",
        "id": 9,
        "last_name": "Fourth",
        "url": "http://localhost:8080/api/users/9",
        "username": "worker4"
      },
      "bug_tracker": null,
      "created_date": "2022-02-16T06:25:48.168000Z",
      "data_chunk_size": 72,
      "data_compressed_chunk_type": "imageset",
      "dimension": "2d",
      "frame_count": 25,
      "guide_id": null,
      "id": 7,
      "issues": {
        "count": 1,
        "url": "http://localhost:8080/api/issues?job_id=7"
      },
      "labels": {
        "url": "http://localhost:8080/api/labels?job_id=7"
      },
      "mode": "interpolation",
      "organization": null,
      "project_id": null,
      "source_storage": null,
      "stage": "annotation",
      "start_frame": 0,
      "state": "in progress",
      "status": "annotation",
      "stop_frame": 24,
      "target_storage": null,
      "task_id": 5,
      "type": "annotation",
      "updated_date": "2022-06-22T09:18:45.296000Z",
      "url": "http://localhost:8080/api/jobs/7"
    },

But in the API request

{'assignee': None,
 'bug_tracker': None,
 'created_date': datetime.datetime(2024, 3, 18, 14, 2, 41, 301446, tzinfo=tzutc()),
 'data_chunk_size': 72,
 'data_compressed_chunk_type': 'imageset',
 'dimension': '2d',
 'frame_count': 4,
 'guide_id': None,
 'id': 32,
 'issues': {'count': 0, 'url': 'http://localhost:8080/api/issues?job_id=32'},
 'labels': {'url': 'http://localhost:8080/api/labels?job_id=32'},
 'mode': 'interpolation',
 'organization': None,
 'project_id': None,
 'source_storage': None,
 'stage': 'annotation',
 'start_frame': 0,
 'state': 'new',
 'status': 'annotation',
 'stop_frame': 24,
 'target_storage': None,
 'task_id': 5,
 'type': 'ground_truth',
 'updated_date': datetime.datetime(2024, 3, 18, 14, 2, 41, 301465, tzinfo=tzutc()),
 'url': 'http://localhost:8080/api/jobs/32'}

I tried doing this multiple times by restarting the server, but the result remained the same.

@zhiltsov-max
Copy link
Contributor

Both admins should have equal access rights for any server objects, they should see the same (all) jobs and tasks.

@Viditagarwal7479
Copy link
Contributor Author

YES!!! I figured it out!
The tests that are previous to the failing tests are creating gt_job corresponding to a task_id. Due to this the current session has gt_job for that task_id but the jobs.json doesn't has any information regarding this.

@zhiltsov-max
Copy link
Contributor

Great news! Please add @pytest.mark.usefixtures("restore_db_per_function") to these tests to fix the problem.

@Viditagarwal7479
Copy link
Contributor Author

Wow, that's it! I am trying to fix this for quite a while and came up with this,

(_, gt_job_del_response) = api_client.jobs_api.destroy(gt_job.id)
assert gt_job_del_response.status == HTTPStatus.NO_CONTENT, "newly created gt_job couldn't be deleted"

was just about to push the changes and saw your message. Thank You ;)

@Viditagarwal7479
Copy link
Contributor Author

test_can_get_gt_job_chunk is still failing after adding

@pytest.mark.usefixtures("restore_db_per_function")

but passes on using

(_, gt_job_del_response) = api_client.jobs_api.destroy(gt_job.id)
assert gt_job_del_response.status == HTTPStatus.NO_CONTENT, "newly created gt_job couldn't be deleted"

@Viditagarwal7479
Copy link
Contributor Author

A gentle reminder towards this PR 😅

@zhiltsov-max
Copy link
Contributor

Hi, yes, it seems like these specific tests have an issue with server data cache, as the comment says. I think we need to think if we can improve these tests somehow to avoid doing hacks.

@Viditagarwal7479
Copy link
Contributor Author

Ahh!
Could you explain this line?

if we don't clean the db, the gt jobs created will be reused, and their ids won't conflict

How about instead of restoring the db this way, we just delete the newly created gt_job through the API call?

@zhiltsov-max
Copy link
Contributor

zhiltsov-max commented Mar 19, 2024

After the comment there is a line that's supposed to restore the DB only when the class tests begin to execute, but not before each test. That is, it's supposed that some test will create a GT job, and the other will be able to reuse it.

How about instead of restoring the db this way, we just delete the newly created gt_job through the API call?

It's certainly doable, but with the current function design we can't be sure the GT job was created. I think, we can change the function to always create such a job. Then, we can use pytest's request.addfinalizer() for cleanup. We'll also need to remove the added restore_db_per_function decorators. We can also add a setup() fixture, but each test required different tasks, depending on the parameters, so it can be more difficult.

@Viditagarwal7479
Copy link
Contributor Author

Ah, request.addfinalizer() this approach seems great to me.
Should I make these changes?

@zhiltsov-max
Copy link
Contributor

Yes, please update the tests relying on optional gt job creation.

@Viditagarwal7479
Copy link
Contributor Author

Viditagarwal7479 commented Mar 19, 2024

EDIT: Like here in the HELM test it didn't failed but keeps failing on my local.

I also observed that test_can_get_gt_job_chunk is non-determinant. It sometimes passes and, at times, fails at original-interpolation (mostly) and rarely at original-annotation.

Here's some output that I recorded when original-interpolation failed,

print(set(chunk.namelist()))
>>> {'000000.jpeg', '000008.jpeg', '000006.jpeg', '000003.jpeg', '000007.jpeg', '000004.jpeg', '000002.jpeg', '000001.jpeg', '000005.jpeg', '000010.jpeg', '000009.jpeg'}
print(set("{:06d}.jpeg".format(i) for i in frame_range))
>>> {'000000.jpeg', '000017.jpeg', '000011.jpeg', '000024.jpeg', '000008.jpeg', '000016.jpeg', '000001.jpeg', '000020.jpeg', '000004.jpeg', '000002.jpeg', '000012.jpeg', '000015.jpeg', '000006.jpeg', '000009.jpeg', '000021.jpeg', '000022.jpeg', '000023.jpeg', '000010.jpeg', '000005.jpeg', '000014.jpeg', '000018.jpeg', '000003.jpeg', '000007.jpeg', '000013.jpeg', '000019.jpeg'}
print(task_meta)
>>> {'chunk_size': 72,
 'deleted_frames': [],
 'frame_filter': '',
 'frames': [{'has_related_context': False,
             'height': 720,
             'name': 'test_video_1.mp4',
             'related_files': 0,
             'width': 1280}],
 'image_quality': 70,
 'included_frames': None,
 'size': 25,
 'start_frame': 0,
 'stop_frame': 24}

Here I suspect that, as per the code here the chunk_size is 72 thus it will select all the frames the task has.

@Viditagarwal7479
Copy link
Contributor Author

It's showing "1 review requesting changes by reviewers with write access" but I have incorporated all the requested changes so far.

@zhiltsov-max zhiltsov-max merged commit dd64046 into cvat-ai:develop Mar 21, 2024
34 checks passed
@zhiltsov-max
Copy link
Contributor

Thank you for contributing into the project!

@Viditagarwal7479
Copy link
Contributor Author

Thank you! For helping me through out.

@Viditagarwal7479 Viditagarwal7479 deleted the tests_gt_job branch March 21, 2024 11:44
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.

Some tests create gt_job for tasks which already have a gt_job
2 participants