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

WIP: adding task input info #556

Merged
merged 28 commits into from
Sep 9, 2022
Merged

WIP: adding task input info #556

merged 28 commits into from
Sep 9, 2022

Conversation

rcali21
Copy link
Contributor

@rcali21 rcali21 commented Jul 18, 2022

Working on adding task info for executables to be printed in the audit module. This includes software tool description and uuid for sys user.

@codecov
Copy link

codecov bot commented Jul 18, 2022

Codecov Report

Base: 77.06% // Head: 77.13% // Increases project coverage by +0.07% 🎉

Coverage data is based on head (40200b3) compared to base (5f5044d).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #556      +/-   ##
==========================================
+ Coverage   77.06%   77.13%   +0.07%     
==========================================
  Files          20       20              
  Lines        4316     4330      +14     
  Branches     1212     1217       +5     
==========================================
+ Hits         3326     3340      +14     
  Misses        802      802              
  Partials      188      188              
Flag Coverage Δ
unittests 77.04% <100.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pydra/engine/audit.py 90.32% <100.00%> (+1.43%) ⬆️
pydra/engine/core.py 89.07% <100.00%> (+0.10%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@djarecka
Copy link
Collaborator

You have a git conflict that you haven't resolved, you either have to keep l. 55-56 or 59.

@rcali21 rcali21 changed the title WIP: adding system user info WIP: adding task input info Aug 24, 2022
@djarecka
Copy link
Collaborator

djarecka commented Aug 29, 2022

This branch still have unresolved message from git conflict that I mentioned in my comments a month ago. It is important to fix it. There is no way that test can pass before fixing it

some resources: https://www.atlassian.com/git/tutorials/using-branches/merge-conflicts

pydra/engine/audit.py Outdated Show resolved Hide resolved
@djarecka
Copy link
Collaborator

@rcali21 - most of the tests are now passing after our yesterday changes. Please check the the report from windows - I've suggested removing one import that you're not using and leads to the error.

the pre-commit test should be fixed if you properly set your python environment and install and run pre-commit (see README)

@djarecka
Copy link
Collaborator

djarecka commented Sep 1, 2022

Hi @rcali21 - were you able to fix the environment and the tests?

@djarecka
Copy link
Collaborator

djarecka commented Sep 4, 2022

@rcali21 - we fixed the code last week and removed all the git additions due to conflicts, but I can see them again, you committed some yesterday (commit b25a84f). They have to be removed again.
In addition this commit include code that we improved last time, e.g. we agreed that the this test can't ever fail:
b5e6c6d#diff-a11f03680cf3564d504176495bedcb6049de9c17cad825a6baa14bb7957685a9R1004

I'm really not sure why this code is here again here and why you had a new conflict, I thought that you will just work on this specific branch. Did you try to merge anything? Did you work on a different computer?

@rcali21
Copy link
Contributor Author

rcali21 commented Sep 4, 2022

Hi @djarecka,

Sorry about this, I tried to resolve the env issues I was having and this is not what I intended to commit. On my end, I have the fixes implemented and the test case is fixed as well. Everything was tested prior to committing and passed. I'm going to try to figure out what the issue is with my branch + env today and get back to you.

Ryan

@rcali21
Copy link
Contributor Author

rcali21 commented Sep 4, 2022

@djarecka This looks to be sorted now, correct?

@djarecka
Copy link
Collaborator

djarecka commented Sep 5, 2022

yes, this looks much better, all pytest test are passing now, but please check the pre-commit report - there is one line that is too long: https://results.pre-commit.ci/run/github/152126811/1662310974.aPQj1izcSX-yLFZRsy_MBQ

@rcali21
Copy link
Contributor Author

rcali21 commented Sep 5, 2022

yes, this looks much better, all pytest test are passing now, but please check the pre-commit report - there is one line that is too long: https://results.pre-commit.ci/run/github/152126811/1662310974.aPQj1izcSX-yLFZRsy_MBQ

Resolved by deleting verbose comment.

pydra/engine/audit.py Outdated Show resolved Hide resolved
pydra/engine/audit.py Outdated Show resolved Hide resolved
@djarecka
Copy link
Collaborator

djarecka commented Sep 6, 2022

I left a couple of comments, once this is fixed we can merge it and start adding input, output or agent in new PR

@djarecka
Copy link
Collaborator

djarecka commented Sep 9, 2022

great, thank you! I will merge this PRs! other fields can be added in a separate PR.

Please be sure that you fetch/pull changes from the master branch and start new changes in a new branch!

@djarecka djarecka merged commit 64916f4 into nipype:master Sep 9, 2022
@djarecka
Copy link
Collaborator

djarecka commented Sep 9, 2022

please also add your name to the zenodo file: https://github.com/nipype/pydra/blob/master/.zenodo.json (you can open a small PR with it)

@rcali21 rcali21 mentioned this pull request Sep 9, 2022
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.

2 participants