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

make the poetry installation work; add dalmatian as a package #18

Open
wants to merge 2 commits into
base: poetry
Choose a base branch
from

Conversation

qinqian
Copy link

@qinqian qinqian commented Jan 26, 2023

We have a working version of poetry for genepy python packages, while ignoring all R packages dependencies. We need to find the necessary functions for depmap_omics to add into that repo

@qinqian qinqian requested a review from pgm January 26, 2023 03:41
@pgm
Copy link
Collaborator

pgm commented Jan 26, 2023

Ah! I didn't fully understand what you meant when you said you added damination, but now that I've looked at the diff, I see you copied the damination into this repo.

I also see bumped the version of python to 3.11.

My guess of what's going on here is:

  • you want to use python 3.11
  • firecloud-damination is broken in python 3.11
  • you copied the damination code into a folder into this repo and made some small fixes to allow it to work with python 3.11

Is that correct?

If so, I propose a slightly different workflow:

  1. use github's "fork" function to make a clone of firecloud-damination called something like cds-firecloud-damination.
  2. Make a branch in the cds-firecloud-damination repo with your python 3.11 fix.
  3. update the dependencies to install dalmaition from that repo instead of from pypi.

this has advantages for maintenance in the future:

  1. If firecloud-dalmatian is updated in the future, we'll be able to easily "merge" their changes into our patched version.
  2. you can provide your 3.11 fix to them as a pull-request. The easier you make it for them to take, the more likely they might just merge it in.
  3. it captures the history better where we'll be able to see that the only difference between our version and their version is just the changes you made. (As opposed to looking like these are entirely new files in this repo)

@qinqian
Copy link
Author

qinqian commented Jan 26, 2023

Yes, it is correct. For the firecloud-damination pypi package used in genepy, it only supports Python 3.4. After a closer look at their latest commit at firecloud-damination github, they already corrected this.

I agree with you on the workflow that will easily update the damination with cds-firecloud-damination fork. I'll make a revision on this poetry pull request using this suggestion

@qinqian qinqian self-assigned this Feb 12, 2023
@qinqian qinqian added the enhancement New feature or request label Feb 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants