Skip to content

[SYNPY-1765] Remove project id arg from create_record_based_curator_task#1329

Merged
andrewelamb merged 11 commits intodevelopfrom
SYNPY-1765
Feb 26, 2026
Merged

[SYNPY-1765] Remove project id arg from create_record_based_curator_task#1329
andrewelamb merged 11 commits intodevelopfrom
SYNPY-1765

Conversation

@andrewelamb
Copy link
Contributor

@andrewelamb andrewelamb commented Feb 24, 2026

Problem:

  • create_record_based_metadata_task required both a folder_id and project_id
  • The description for record_set_name in create_record_based_metadata_task and for RecordSet needed improvement.

Solution:

  • Added project_id_from_entity_id function.
  • Use of project_id is deprecated and no longer used. Now project_id is gotten from the folder_id.
  • Docstrings have been improved with the provided descriptions in the Jira ticket.
  • File based task creation also uses project_id_from_entity_id function

Testing:

  • Integration/unit tests for project_id_from_entity_id function
  • project_id_from_entity_id function has been patched in existing unit tests for create_record_based_metadata_task

@andrewelamb andrewelamb requested a review from a team as a code owner February 24, 2026 21:35
@andrewelamb andrewelamb marked this pull request as draft February 24, 2026 21:35
@andrewelamb andrewelamb marked this pull request as ready for review February 24, 2026 21:45
@thomasyu888 thomasyu888 requested a review from linglp February 25, 2026 00:03
@andrewelamb andrewelamb marked this pull request as draft February 25, 2026 00:22
@andrewelamb andrewelamb marked this pull request as ready for review February 25, 2026 16:26
Copy link
Contributor

@linglp linglp left a comment

Choose a reason for hiding this comment

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

Hi @andrewelamb ! This is looking really close! A few remaining items:

Documentation that needs to be fixed:

  • synapseclient/extensions/curator/readme.md see: create_record_based_metadata_task
    *synapsePythonClient/docs/guides/extensions/curator/metadata_curation.md see create_record_based_metadata_task

Also, the projects created by the integration test should have unique names otherwise we might trigger a racing condition.

Also, do you think project_id_from_entity_id belongs in a util module?

@andrewelamb
Copy link
Contributor Author

andrewelamb commented Feb 26, 2026

Also, do you think project_id_from_entity_id belongs in a util module?

I'm happy with this. Do you have a suggested location? synapseclient/core/utils.py ?

@andrewelamb andrewelamb requested a review from linglp February 26, 2026 18:03
@linglp
Copy link
Contributor

linglp commented Feb 26, 2026

Also, do you think project_id_from_entity_id belongs in a util module?

I'm happy with this. Do you have a suggested location? synapseclient/core/utils.py ?

How about creating a util module for the curator extension, since only the curator extension needs it at the moment

@andrewelamb andrewelamb marked this pull request as draft February 26, 2026 19:26
@andrewelamb andrewelamb marked this pull request as ready for review February 26, 2026 19:33
while not isinstance(current_obj, Project):
current_obj = get(current_obj.parent_id, synapse_client=synapse_client)
iterations += 1
if iterations > 1000:
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit curious why you picked 1000 as the magic number? should we also encode that on top of the Python file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's a good question. I figured it was a big enough number that there's no way a DCC would actually nest folders to that degree, but was also small enough that if there was actually something wrong with Synapse it wouldn't loop forever. I'm open to changing it, though!

I can set it as a global.

@andrewelamb andrewelamb requested a review from linglp February 26, 2026 20:07
Copy link
Contributor

@linglp linglp left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the changes.

@andrewelamb andrewelamb merged commit dcda6d0 into develop Feb 26, 2026
21 checks passed
@andrewelamb andrewelamb deleted the SYNPY-1765 branch February 26, 2026 21:02
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.

3 participants