ARR Commitment: get previous url and allow identity visibility #2583
Conversation
… of arr submissions
There was a problem hiding this comment.
Pull Request Overview
This PR expands the ARR commitment processing by updating the process_commitment_venue function to allow for handling previous URL submissions and enabling identity visibility within the ARR workflow, and it also adds a new utility to rename venue domains.
- In tests/test_arr_venue_v2.py, the test is updated to trigger the new functionality by providing get_previous_url_submission=True and identity_visibility=True.
- In openreview/openreview.py, a new rename_domain method is introduced to update an entire venue’s domain.
- In openreview/arr/arr.py, the process_commitment_venue function is refactored with additional parameters and helper functions (like update_deanonymizers and process_previous_url) to support the new features.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_arr_venue_v2.py | Updates the ARR commitment test to include the new parameters for processing submissions. |
| openreview/openreview.py | Adds a new rename_domain method along with an attribute for domain renaming support. |
| openreview/arr/arr.py | Enhances process_commitment_venue with new parameters and helper functions to process previous URLs and update deanonymizers. |
Comments suppressed due to low confidence (2)
tests/test_arr_venue_v2.py:5958
- Ensure that the additional behavior when get_previous_url_submission and identity_visibility are true is adequately tested, and consider adding tests for scenarios when they are false.
openreview.arr.ARR.process_commitment_venue(openreview_client, 'aclweb.org/ACL/ARR/2024/Workshop/C3NLP_ARR_Commitment', get_previous_url_submission=True, identity_visibility=True)
openreview/arr/arr.py:642
- [nitpick] Consider renaming 'group_id' to a more descriptive name (e.g., 'venue_id' or 'domain') to clarify its role in updating deanonymizers.
def update_deanonymizers(group, group_id, commitment_readers_group_id):
| @@ -592,6 +593,29 @@ def post_profile(self, profile): | |||
| response = self.__handle_response(response) | |||
| return Profile.from_json(response.json()) | |||
|
|
|||
| def rename_domain(self, old_domain, new_domain): | |||
There was a problem hiding this comment.
The docstring for rename_domain indicates that it returns a status dict tracking the process, but the implementation returns a Profile object. Update either the implementation or the docstring to ensure consistency.
Copilot uses AI. Check for mistakes.
| @@ -629,6 +639,21 @@ def create_readers_group(submission, original_submission): | |||
| ) | |||
| ) | |||
|
|
|||
| def update_deanonymizers(group, group_id, commitment_readers_group_id): | |||
| if group: | |||
| deanonymizers = getattr(group, 'deanonymizers', []) | |||
There was a problem hiding this comment.
the way to check if a group has deanonymizers is group.anonids is True.
if group.anonids:
deanonymizers = group.deanonymizers
...change deanonymizers
else:
readers = group.readers
...change readers
| # This adds the Commitment readers group to the deanonymizer for the assigned AC and reviewers group of the ARR submission so the commitment ACs can see the identities of the reviewers and ACs | ||
| if identity_visibility: | ||
| # Update Area Chairs group | ||
| arr_ac_group = client.get_group(f'{domain}/Submission{submission.number}/Area_Chairs') |
There was a problem hiding this comment.
are we assuming that all the ARR submissions use "Area Chairs" and "Reviewers" as committee role names?
There was a problem hiding this comment.
That is assumed because it hasn't changed, but I could parameterize it in case.
There was a problem hiding this comment.
@melisabok I asked Harold and he said 'some of the old forms might reference "Action Editors" and "Senior Action Editors", and some invitations are still named that but the groups themselves were always Area_Chairs and Senior_Area_Chairs'
| previous_url_id = previous_url.split('=')[-1] | ||
| previous_url_submission = openreview.tools.get_note(client, previous_url_id) | ||
| if previous_url_submission: | ||
| create_readers_group(previous_url_submission, arr_submission) |
There was a problem hiding this comment.
don't you need to pass the commitment note instead of the arr_submission here?
There was a problem hiding this comment.
The previous_url exists in the arr submission and not the commitment. the commitment only has paper_link
| @@ -5955,7 +5955,7 @@ def test_commitment_venue(self, client, test_client, openreview_client, helpers) | |||
| notes = pc_client_v2.get_notes(invitation='aclweb.org/ACL/ARR/2023/August/-/Submission', number=3) | |||
| assert len(notes) == 0 | |||
|
|
|||
| openreview.arr.ARR.process_commitment_venue(openreview_client, 'aclweb.org/ACL/2024/Workshop/C3NLP_ARR_Commitment') | |||
| openreview.arr.ARR.process_commitment_venue(openreview_client, 'aclweb.org/ACL/2024/Workshop/C3NLP_ARR_Commitment', get_previous_url_submission=True, identity_visibility=True) | |||
There was a problem hiding this comment.
I would like you verify the changes are working here. There must be an ARR august submission that has a previous url, you need to assert that aclweb.org/ACL/2024/Workshop/C3NLP_ARR_Commitment has access to the previous submission and the identity of the reviewers ACs.
|
@racheljsmart any update on this? if you are too busy I can help to finish it. |
update_deanonymizers() and process_previous_url() were added to process_commitment_venue().
update_deanonymizers is triggered if identity_visibility is True and it's executed when readers are added to arr submissions in add_readers_to_arr_submission().
process_previous_url() is triggered in process_commitment_submission if get_previous_url_submission is True.
I've also added a description to process_commitment_venue().
I want to test it a more extensively, but I would like feedback before spending more time on it.