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

refactor of pipeline #52

Merged
merged 20 commits into from
Oct 15, 2020
Merged

refactor of pipeline #52

merged 20 commits into from
Oct 15, 2020

Conversation

jbloom
Copy link
Member

@jbloom jbloom commented Oct 14, 2020

@dbacsik, this is my pull request that refactors the pipeline. If this looks good to you, we can merge it into main and then work from there. Lots of changes! Main ones:

  • Experiments are now specified in config.yaml as you are familiar with.
  • Jupyter notebooks are now run using the snakemake capability for this (see use new Snakemake Jupyter notebook integration #46).
  • I have removed various unneeded stuff.
  • The report.html file is now written to results and isn't tracked for now as it gets too many differences.

Note that this does not have the analysis notebooks, so we need to add those. But I thought we could try to get this on main and then make future pull requests to it to consolidate the other branches back in. Does that sound good?

@dbacsik
Copy link
Collaborator

dbacsik commented Oct 14, 2020

@jbloom That sounds like a good strategy. We can add back the analysis notebooks one at a time and review them.

I am going to make sure this runs on my local directory before I approve the pull request. I've been getting an error that I think is related to the new snakemake/jupyter integration. I will try this new version and see if it resolves the issue.

@dbacsik
Copy link
Collaborator

dbacsik commented Oct 15, 2020

This looks good. The jupyter-nbconvert issue was fixed by changing the permissions. The pipeline runs without error and the output that I spot checked looks good.

@dbacsik dbacsik closed this Oct 15, 2020
@dbacsik dbacsik reopened this Oct 15, 2020
Copy link
Collaborator

@dbacsik dbacsik left a comment

Choose a reason for hiding this comment

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

Ready to merge.

@dbacsik dbacsik merged commit 706123e into main Oct 15, 2020
@jbloom jbloom deleted the new_refactor_pipeline branch October 15, 2020 18:39
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.

None yet

2 participants