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

Consensus simple merging #8953

Merged
merged 149 commits into from
Feb 21, 2025
Merged

Consensus simple merging #8953

merged 149 commits into from
Feb 21, 2025

Conversation

zhiltsov-max
Copy link
Contributor

@zhiltsov-max zhiltsov-max commented Jan 16, 2025

Motivation and context

Depends on #8939, #9026
A part of #8434

  • Added support for consensus task and consensus job merging (API and UI)
  • Added simple consensus settings
  • Added server tests
  • Added new consensus RQ queue and worker
  • Updated skeleton comparisons: hidden points now also contribute to the skeleton similarity. Only visibility is taken into account for invisible points

Limitations:

  • Merging is supported for all annotations except 2d and 3d cuboids. 3d tasks are not supported
  • Annotation groups are not supported (each annotation is considered separate in a group)
  • Polygons and masks are not interchangeable (each type is compared only with the same type)

How has this been tested?

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.

@@ -148,13 +148,16 @@ def parse_exception_message(msg: str) -> str:
pass
return parsed_msg

def process_failed_job(rq_job: Job):
def process_failed_job(rq_job: Job, *, logger: logging.Logger | None = None) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do this? I think we should just let each module log with its own logger.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly to allow each module to use the corresponding logger. It was a fixed logger before.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean... the module is also fixed. It's cvat.apps.engine.utils. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function is currently used from consensus and from engine. Consensus passes it's own logger. How would you like it to be changed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep the function the same as it was before. ('cvat.server.engine' ought to be replaced by __name__, but that's unrelated to this PR.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then consensus will print errors into the engine logger.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the problem with that? All the logger does is identify the module that emitted the message. In this case, the module is cvat.apps.engine.utils.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that the produced log messages are both misleading (the module name) and uninformative (no full stacktrace to the message source):

[2025-02-21 14:53:11,412] ERROR cvat.server.engine: Traceback (most recent call last):
  File "/opt/venv/lib/python3.10/site-packages/rq/worker.py", line 1431, in perform_job
    rv = job.perform()
  File "/opt/venv/lib/python3.10/site-packages/rq/job.py", line 1280, in perform
    self._result = self._execute()
  File "/opt/venv/lib/python3.10/site-packages/rq/job.py", line 1317, in _execute
    result = self.func(*self.args, **self.kwargs)
  File "/home/django/cvat/apps/profiler.py", line 12, in wrapped
    return f(*args, **kwargs)
  File "/home/django/cvat/apps/consensus/merging_manager.py", line 249, in _merge
    return _TaskMerger(task=job.get_task_id()).merge_single_consensus_job(target_id)
  File "/usr/lib/python3.10/contextlib.py", line 79, in inner
    return func(*args, **kwds)
  File "/home/django/cvat/apps/consensus/merging_manager.py", line 157, in merge_single_consensus_job
    self._merge_consensus_jobs(parent_job_id)
  File "/home/django/cvat/apps/consensus/merging_manager.py", line 102, in _merge_consensus_jobs
    raise Exception("test")
Exception: test

Another problem I see is that they also may expose sensitive information, as they are returned from the API for any exception type and show the full stacktrace.

I decided to revert the change, because improvements require a separate PR and fixing just the logger name doesn't change the situation significantly.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
C Security Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@SpecLad SpecLad merged commit 4d06ae1 into develop Feb 21, 2025
33 of 34 checks passed
@SpecLad SpecLad deleted the zm/consensus-simple-merging branch February 21, 2025 16:41
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.

4 participants