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

change people functions to synapser #168

Merged
merged 3 commits into from
Feb 23, 2024
Merged

change people functions to synapser #168

merged 3 commits into from
Feb 23, 2024

Conversation

allaway
Copy link
Collaborator

@allaway allaway commented Feb 16, 2024

This PR introduces synapser to Imports and refactors the add_people_from_table and .store_rows functions to use synapser. Modified the .check_login function to look for syn object imported by library(synapser).

@allaway allaway requested a review from anngvu February 16, 2024 19:17
@allaway
Copy link
Collaborator Author

allaway commented Feb 16, 2024

@anngvu , any ideas on why it would fail to build this package with synapser on mac and windows but work fine on ubuntu?

@anngvu
Copy link
Contributor

anngvu commented Feb 20, 2024

@allaway Since I'm mainly Linux, I'm honestly sure what the Mac and Windows issues are -- the logs aren't super helpful -- will need to look more into it.

@anngvu
Copy link
Contributor

anngvu commented Feb 20, 2024

@allaway OK, I think this is actually a GH workflow issue.

* Respecify OS versions for Mac and Windows

* Add fallback deps install

* Upgrade step version
@anngvu
Copy link
Contributor

anngvu commented Feb 22, 2024

So testing locally:

  • On Sage Mac laptop, building synapser when installing with pak was not an issue.
  • On borrowed Windows laptop, couldn't build synapser when installing with pak and there was just not enough information to resolve.

So I revised the workflow to use precompiled synapser from Synapse repo when building from source fail for those OS platforms -- this corresponds with the official installation instructions and is how most users would obtain synapser in any case.

Copy link
Contributor

@anngvu anngvu left a comment

Choose a reason for hiding this comment

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

I think a hint of what source table/column usually maps to in the NF context would be nice, but as functionality goes everything works with people_table_id = "syn23564971", people_column = "ownerID", source_table_id = "syn16858331", source_column = "createdBy", both dry_run TRUE and FALSE, so merging.

@anngvu anngvu merged commit 4320d35 into develop Feb 23, 2024
7 checks passed
anngvu added a commit that referenced this pull request Jul 15, 2024
allaway pushed a commit that referenced this pull request Jul 17, 2024
* Revert "change people functions to synapser (#168)"

This reverts commit 4320d35.

* Update README
@anngvu anngvu deleted the add-synapser branch July 25, 2024 18:12
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.

Refactor synapse-adjacent functions to use new reticulate-based synapser
2 participants