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.

Sign up for GitHub

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

Jump to bottom

Implement promised requirements (resolves #729) #828

Merged
merged 3 commits into from
Jun 8, 2016
Merged

Implement promised requirements (resolves #729) #828

merged 3 commits into from
Jun 8, 2016

Conversation

jpfeil
Copy link
Contributor

@jpfeil jpfeil commented Apr 28, 2016

Resolves #729, continued from #791

@jpfeil jpfeil mentioned this pull request Apr 28, 2016
Closed
@hannes-ucsc
Copy link
Member

Add a pinch of dill …

… to the list of requirements in setup.py

@jpfeil jpfeil force-pushed the issues/729-promised-requirement branch 6 times, most recently from e9de509 to c6cb9ee Compare May 2, 2016 21:41
@hannes-ucsc hannes-ucsc self-assigned this May 9, 2016
@jpfeil jpfeil force-pushed the issues/729-promised-requirement branch 2 times, most recently from f0ac356 to 1652568 Compare May 10, 2016 00:18
@hannes-ucsc hannes-ucsc modified the milestone: 3.2.0 release May 10, 2016
@jpfeil jpfeil force-pushed the issues/729-promised-requirement branch from 9a7de83 to c8cd2b6 Compare May 17, 2016 17:31
@jpfeil
Copy link
Contributor Author

jpfeil commented May 18, 2016

Jenkins, test this please

@hannes-ucsc
Copy link
Member

Merged BD2KGenomics/cgcloud#167 and created and registered a new toil-jenkins-slave image.

Jenkins, test this please.

Fingers crossed.

@hannes-ucsc
Copy link
Member

Master was bjørked. Retrying …

Jenkins, test this please.

@jpfeil jpfeil removed the needs work label May 19, 2016
@jpfeil
Copy link
Contributor Author

jpfeil commented May 19, 2016

@hannes-ucsc

hannes-ucsc
hannes-ucsc reviewed May 19, 2016
View reviewed changes
src/toil/job.py
return func(*self._args)


def convertPromiseToPromisedRequirement(kwargs):
Copy link
Member

@hannes-ucsc hannes-ucsc May 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Combine these two methods. It should be possible to convert any promises and then return True if any conversions were made.

Also make the resulting function a static method of the PromisedRequirement class and rename it to just convertPromises.

@jpfeil jpfeil force-pushed the issues/729-promised-requirement branch 4 times, most recently from a554c2b to 8d1c89a Compare May 20, 2016 02:19
@jpfeil
Copy link
Contributor Author

jpfeil commented May 20, 2016

@hannes-ucsc

@jpfeil jpfeil removed the needs work label May 20, 2016
@benedictpaten
Copy link
Contributor

@jpfeil please address the issues regarding comments and docs we discussed today and I think we can merge this.

@hannes-ucsc
Copy link
Member

@benedictpaten, I've set the needs work label to reflect your previous statement

@hannes-ucsc
Copy link
Member

@jpfeil, @benedictpaten : Did you guys agree to add a Sphinx document section?

@jpfeil
Copy link
Contributor Author

jpfeil commented May 25, 2016

@hannes-ucsc Yes

@jpfeil jpfeil force-pushed the issues/729-promised-requirement branch 2 times, most recently from 6b022b2 to bb3ef36 Compare May 26, 2016 00:04
@jpfeil jpfeil removed the needs work label Jun 7, 2016
@jpfeil
Copy link
Contributor Author

jpfeil commented Jun 7, 2016

@hannes-ucsc

@hannes-ucsc
Copy link
Member

This has conflicts, I can't review until those are resolved.

@jpfeil jpfeil force-pushed the issues/729-promised-requirement branch 2 times, most recently from 1c88dd7 to e4a2161 Compare June 8, 2016 01:33
@hannes-ucsc
Copy link
Member

Looks like the tests failures are directly related to this PR. Leaving as needs work.

@jpfeil jpfeil force-pushed the issues/729-promised-requirement branch from e4a2161 to 9ae4eb5 Compare June 8, 2016 04:34
@jpfeil
Copy link
Contributor Author

jpfeil commented Jun 8, 2016

@hannes-ucsc

@jpfeil jpfeil removed the needs work label Jun 8, 2016
@hannes-ucsc hannes-ucsc merged commit 1195eff into master Jun 8, 2016
hannes-ucsc
hannes-ucsc reviewed Jun 9, 2016
View reviewed changes
setup.py
'bd2k-python-lib==1.13.dev14'],
'bd2k-python-lib==1.13.dev14',
'dill==0.2.5'],
tests_require=[
Copy link
Member

Choose a reason for hiding this comment

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

BTW, I didn't catch this earlier but this is a merge error on your part, @jpfeil. You reintroduced lines that I had taken out. Try to be extra careful with merges!

Copy link
Member

Choose a reason for hiding this comment

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

… sorry, with resolving merge conflicts.

@ejacox ejacox deleted the issues/729-promised-requirement branch September 21, 2017 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@hannes-ucsc hannes-ucsc

Labels
None yet
Projects
None yet
Milestone
Sprint 03 (3.2.0)
Development

Successfully merging this pull request may close these issues.
3 participants
@jpfeil @hannes-ucsc @benedictpaten