fixing some tests - Githubissues

fidelity / spock

spock is a framework that helps manage complex parameter configurations during research and development of Python applications

https://fidelity.github.io/spock/

Apache License 2.0

123 stars 13 forks source link

fixing some tests #163

Closed gbmarc1 closed 2 years ago

gbmarc1 commented 2 years ago

name: Pull request about: Create a pull request for merge

What does this PR do?

E.g. Describe the added feature or what issue it fixes #(issue)...

Checklist

[ ] Did you adhere to PEP-8 standards?
[ ] Did you run black and isort prior to submitting your PR?
[ ] Does your PR pass all existing unit tests?
[ ] Did you add associated unit tests for any additional functionality?
[ ] Did you provide documentation (Numpy Docstring format) whenever possible, even for simple functions or classes?

Review

Request will go to reviewers to approve for merge.

gbmarc1 commented 2 years ago

@ncilfone There is my work to fix the dag nesting issue. It was a big refactor but I think it was worthwhile in terms of code simplicity and clarity. Hope you like the new code.

All tests are now passing. Test were not passing on windows at first because of path issues. This PR also contains those fixes for windows users

ncilfone commented 2 years ago

@gbmarc1 I'm back from a bit of R&R. Will take a look through all of this as soon as I can!

ncilfone commented 2 years ago

@gbmarc1 First pass through looks pretty great!

My two concerns are:

Use of dataclasses breaks Python 3.6 compat (there is a dep you can add to get this back in -- but I'd like to avoid if possible). There should be a way to refactor that out since it's only used in one place I can see...
networkx is a pretty hefty dep seeing as the only two graph ops we are doing is a topological sort and cycle checking (via DFS) -- unless I'm seeing it wrong.

I'm going to try and refactor a mash-up of the two PRs to see if we keep the lean deps and python version support while adding the cleaner backend you refactored into...

gbmarc1 commented 2 years ago

@gbmarc1 First pass through looks pretty great!

My two concerns are:

Use of dataclasses breaks Python 3.6 compat (there is a dep you can add to get this back in -- but I'd like to avoid if possible). There should be a way to refactor that out since it's only used in one place I can see...

networkx is a pretty hefty dep seeing as the only two graph ops we are doing is a topological sort and cycle checking (via DFS) -- unless I'm seeing it wrong.

I'm going to try and refactor a mash-up of the two PRs to see if we keep the lean deps and python version support while adding the cleaner backend you refactored into...

We can easily remove the dataclasses decorator and use a normal class instead. For netowrkx, I agree that we dont use it that much. Indeed, it only verifies that there is no cycle in the graph and we are able to get the roots easily. However, networkx is a pretty standard library. It's up to you.

ncilfone commented 2 years ago

Merged via #175

ncilfone commented 2 years ago

@gbmarc1 Apologies as I think my PR (#175) of your PR of your fork (instead of pushing back to your PR and fork) removed all of your attribution of the code in the git blame. I didn't notice this until just now.

We can roll back if you'd like attribution in the blame? If not I've got a coauthor tag in the PR with your name (Co-authored-by: mbelanger_explorance mbelanger@explorance.com) to make sure you get due attribution to your efforts.

Lmk and I'm happy to remedy my screw-up with some PR rollbacks to make sure you are part of the blame (and contributors)