-
Notifications
You must be signed in to change notification settings - Fork 781
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
GitHub: Add bi-directional sync #3565
base: master
Are you sure you want to change the base?
Conversation
apps/challenges/github_utils.py
Outdated
URLS = {"contents": "/repos/{}/contents/{}", "repos": "/repos/{}"} | ||
|
||
|
||
class Github_Interface: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename class to GithubInterface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
apps/challenges/github_utils.py
Outdated
return None | ||
return response.json() | ||
|
||
def return_github_url(self, url): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename the method to get_github_url
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
apps/challenges/github_utils.py
Outdated
url = "{0}{1}".format(base_url, url) | ||
return url | ||
|
||
def get_content_from_path(self, path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add docstring for this method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
apps/challenges/github_utils.py
Outdated
return response | ||
|
||
def get_data_from_path(self, path): | ||
content_response = self.get_content_from_path(path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
apps/challenges/github_utils.py
Outdated
return string_data | ||
|
||
def update_content_from_path(self, path, content): | ||
url = URLS.get("contents").format(self.GITHUB_REPOSITORY, path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
apps/challenges/github_utils.py
Outdated
|
||
@app.task | ||
def github_challenge_phase_sync(challenge_phase): | ||
from .serializers import ChallengePhaseSerializer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move to global imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it is a circular import.
apps/challenges/github_utils.py
Outdated
from .serializers import ChallengePhaseSerializer | ||
|
||
for obj in serializers.deserialize("json", challenge_phase): | ||
challenge_phase_obj = obj.object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create a util method for deserializing and use that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
apps/challenges/github_utils.py
Outdated
"is_restricted_to_select_one_submission", | ||
"is_partial_submission_evaluation_enabled", | ||
"allowed_submission_file_types", | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move to a common util file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
apps/challenges/github_utils.py
Outdated
github.update_data_from_path("challenge_config.yaml", content_str) | ||
|
||
# File fields Update | ||
file_fields = ["description"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
def github_sync_challenge(sender, instance, created, **kwargs): | ||
if instance.github_repository and instance.github_token: | ||
serialized_obj = serializers.serialize("json", [instance]) | ||
github_challenge_sync.delay(serialized_obj) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@savish28 the method names are confusing github_challenge_sync
and github_sync_challenge
(post save hook) it is not clear what these methods are for. Please rename the methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
@RishabhJain2018 Yes it is!! |
@@ -0,0 +1,37 @@ | |||
# Fields from Challenge, ChallengePhase model to be considered for github_sync | |||
|
|||
challenge_non_file_fields = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@savish28 is there a way we can use the serializer or model to generate this list of fields? Currently, if a new field gets added we'll have to add it here manually. It might happen that some forgets adding it here which would mean there is no sync happening for that field. If we can use models/serializer for this list it would be great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ram81, we only wish to have only selected fields(that are editable from frontend) in the sync and not all fields, getting it from serializer would mean using all fields and that is undesirable because there are a lot of fields that are not in the challenge_config and might also be critical to share those.
apps/challenges/github_utils.py
Outdated
URLS = {"contents": "/repos/{}/contents/{}", "repos": "/repos/{}"} | ||
|
||
|
||
class GithubInterface: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@savish28 can we move this class to a separate file called github_interface.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sure :) Done.
apps/challenges/github_utils.py
Outdated
if not github.is_repository(): | ||
return | ||
try: | ||
# Challenge Non-file field Update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to Challenge non-file field update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
apps/challenges/github_utils.py
Outdated
content_str = yaml.dump(challenge_config_yaml, sort_keys=False) | ||
github.update_data_from_path("challenge_config.yaml", content_str) | ||
|
||
# Challenge File fields Update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to Challenge file fields update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
apps/challenges/github_utils.py
Outdated
continue | ||
github.update_data_from_path(field_path, getattr(challenge, field)) | ||
except Exception as e: | ||
logger.info("Github Sync unsuccessful due to {}".format(e)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a info log? This should be a error log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
apps/challenges/github_utils.py
Outdated
field_path, getattr(challenge_phase, field) | ||
) | ||
break | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
github.update_data_from_path( | ||
field_path, getattr(challenge_phase, field) | ||
) | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we have a break here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because at a time only one challenge_phase is updated. Because it is initiated by post-hook of ChallengePhase
apps/challenges/github_utils.py
Outdated
break | ||
|
||
except Exception as e: | ||
logger.info( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, Why is this not a error log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
apps/challenges/models.py
Outdated
@@ -226,6 +231,13 @@ def create_eks_cluster_for_challenge(sender, instance, created, **kwargs): | |||
aws.challenge_approval_callback(sender, instance, field_name, **kwargs) | |||
|
|||
|
|||
@receiver(signals.post_save, sender="challenges.Challenge") | |||
def challenge_sync(sender, instance, created, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to challenge_details_sync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
apps/challenges/models.py
Outdated
@@ -347,6 +359,16 @@ def save(self, *args, **kwargs): | |||
return challenge_phase_instance | |||
|
|||
|
|||
@receiver(signals.post_save, sender="challenges.ChallengePhase") | |||
def challenge_phase_sync(sender, instance, created, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change to challenge_phase_details_sync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
@Ram81 Ping! :) |
Thanks for the ping @savish28. I will review this PR today |
Context: Github Bi-directional Sync
Related Pull Request: Cloud-CV/EvalAI-Starters#52
Models Considered:
Deliverables:
Testing without celery:
https://user-images.githubusercontent.com/32800267/129727539-80d18fd0-7cdc-489d-a3c6-0b346ce605c8.mp4
@KhalidRmb
cc: @Ram81