-
Notifications
You must be signed in to change notification settings - Fork 0
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
Updated Snakemake Formatting #120
Conversation
There is a small rounding difference happening in the
|
According to https://snakemake.readthedocs.io/en/stable/snakefiles/deployment.html, Snakemake recommends a particular file organization structure (for example, the snakefile and envs are in a folder called 'workflow'). Do you think we should structure the codebase more closely to this, or even in the exact suggested way? |
Good point, and its part of the update coming ! |
- ca-certificates | ||
- openssl | ||
- yaml | ||
- pyyaml |
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.
Not sure why openpyxl, ca-certificates, openssl, pyyaml are necessary 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.
Those are just the dependencies it spat out at me that I needed. I started with a fresh environment and only added packages when it asked to try and get the bare minimum. pyyaml
was needed to get past reading from the config.yaml
file (not sure how its different from regular yaml
tbh). Not sure about ca-certificates
and openssl
. Does it run without them?
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.
Hmm... could you check in the error message to see if it says where the dependencies are needed? They don't seem to be required for me when I remove them and run snakemake all
Edit: Never mind, these are required as you pointed out.
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.
your using the --use-conda
flag as well, right?
dependencies: | ||
- pip | ||
- pip: | ||
- otoole==0.11.0 |
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.
As it is, it looks like the model will throw an error if someone has a more recent (>0.11.0) otoole version. If exact otoole version 0.11.0 is recommended to generate the data for the Canada-US model, it should probably be specified in a README somewhere as well.
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.
Is there a newer version then 0.11.0? I think I (successfully) tested it on 0.10.0 as well, cause there is sometime backwards compatibility issues. Nevertheless, good idea! Did you want to add it in?
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.
It does seem to work with otoole 0.10.0, and I don't know why. It's odd that we'd specify a version if it works with older versions anyways, but it's functional, so I think we can leave it be, if you think it's okay.
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.
Its just for when you create the conda environment using the --use-conda
flag
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.
The changes look good/interesting. I've left some questions throughout where I think changes could maybe be made, but it all runs properly.
Updated the snakefile to match the documentation suggested standards (ie. it now passes
snakemake --lint
), and removed any non-used import statements from the modulesIncluded in the update: