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

story/VOGRE-7 #15

Open
wants to merge 114 commits into
base: develop
Choose a base branch
from
Open

story/VOGRE-7 #15

wants to merge 114 commits into from

Conversation

Girik1105
Copy link
Contributor

@Girik1105 Girik1105 commented Nov 5, 2024

Guidelines for Pull Requests

If you haven't yet read our code review guidelines, please do so, You can find them here.

Please confirm the following by adding an x for each item (turn [ ] into [x]).

  • I have removed all code style changes that are not necessary (e.g. changing blanks across the whole file that don’t need to be changed, adding empty lines in parts other than your own code)
  • I am not making any changes to files that don’t have any effect (e.g. imports added that don’t need to be added)
  • I do not have any sysout statements in my code or commented out code that isn’t needed anymore
  • I am not reformatting any files in the wrong format or without cause.
  • I am not changing file encoding or line endings to something else than UTF-8, LF
  • My pull request does not show an insane amount of files being changed although my ticket only requires a few files being changed
  • I have added Javadoc/documentation where appropriate
  • I have added test cases where appropriate
  • I have explained any part of my code/implementation decisions that is not be self-explanatory

Please provide a brief description of your ticket

Get rid of celery and submit quaduples on request
Description

Currently, quadruples are submitted via Celery on a cron job. Celery should be completely removed. Instead a project should have a button that will submit quadrupes to Quadriga. This ticket includes multiple tasks:
Quadruples need to have a flag (if they don’t already) that indicates if they ahve been submitted or not
Quaduples need to be submitted to the new Quadriga 2.0
Only quadruples with complete relationships (in which concepts have been resolved and added to conceptpower) should be submitted.

VOGRE-7

Are there any other pull requests that this one depends on?

VOGRE-14

Anything else the reviewer needs to know?

Since VOGRE-14 has url changes which also affect annotator and annotator factory this PR will require changes once VOGRE-14 merged.

Girik1105 and others added 30 commits August 9, 2024 17:59
…ory text import for importing and repository text for details
@Girik1105 Girik1105 mentioned this pull request Nov 19, 2024
9 tasks
@Girik1105 Girik1105 marked this pull request as draft November 21, 2024 16:36
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the right approach. It is likely to cause memory issues once there are sufficiently many relationsets. It also seems unnecessary. The only time we care about the readiness of a relation to be submitted is when we want to submit them or maybe when we look at a text. So it really only needs to be checked when the page shows all relations, and then only the relations that are relevant (not all of them).

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 think this is the right approach. It is likely to cause memory issues once there are sufficiently many relationsets. It also seems unnecessary. The only time we care about the readiness of a relation to be submitted is when we want to submit them or maybe when we look at a text. So it really only needs to be checked when the page shows all relations, and then only the relations that are relevant (not all of them).

If we only show the relations that can be submitted, then the user will not be able to see all relations that they have created on the text. Can we still show the relations that cannot be submitted with the disabled checkbox? I'll change the way we check for ready relations.

}
}


Copy link
Member

Choose a reason for hiding this comment

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

these two functions are specific to the quadriga api, aren't they? They should not be in the annotations util file.

return Response({'error': 'You are not authorized to submit this RelationSet.'},
status=status.HTTP_403_FORBIDDEN)

if relationset.status != 'ready_to_submit':
Copy link
Member

Choose a reason for hiding this comment

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

this still

return Response({'error': 'You are not authorized to submit this RelationSet.'},
status=status.HTTP_403_FORBIDDEN)

if relationset.status != 'ready_to_submit':
Copy link
Member

Choose a reason for hiding this comment

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

and everywhere else where these strings are used

# logger.debug("Resolve concept failed: %s" % str(E))
# return
# logger.debug("Resolved concept %s" % manager.instance.uri)
pass
Copy link
Member

Choose a reason for hiding this comment

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

remove commented out code

@@ -1,5 +1,4 @@
from __future__ import absolute_import

# This will make sure the app is always imported when
# Django starts so that shared_task will use this app.
from .celery import app as celery_app
# Django starts so that shared_task will use this app.
Copy link
Member

Choose a reason for hiding this comment

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

are these comments still valid?

@jdamerow
Copy link
Member

Resolve conflicts please

@jdamerow jdamerow closed this Nov 28, 2024
@Girik1105 Girik1105 reopened this Dec 11, 2024
@diging-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@jdamerow
Copy link
Member

Resolve conflicts please

@jdamerow jdamerow closed this Dec 12, 2024
@Girik1105 Girik1105 reopened this Dec 13, 2024
@Girik1105 Girik1105 marked this pull request as ready for review December 13, 2024 15:40
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