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

fixing some tests #163

Merged
merged 13 commits into from
Dec 13, 2021
Merged

fixing some tests #163

merged 13 commits into from
Dec 13, 2021

Conversation

gbmarc1
Copy link
Contributor

@gbmarc1 gbmarc1 commented Nov 19, 2021

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
Copy link
Contributor Author

gbmarc1 commented Nov 23, 2021

@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
Copy link
Contributor

ncilfone commented Dec 7, 2021

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

@ncilfone ncilfone self-requested a review December 7, 2021 20:39
@ncilfone ncilfone added the enhancement New feature or request label Dec 7, 2021
@ncilfone
Copy link
Contributor

ncilfone commented Dec 7, 2021

@gbmarc1 First pass through looks pretty great!

My two concerns are:

  1. 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...

  2. 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
Copy link
Contributor Author

gbmarc1 commented Dec 7, 2021

@gbmarc1 First pass through looks pretty great!

My two concerns are:

  1. 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...
  2. 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
Copy link
Contributor

Merged via #175

@ncilfone ncilfone closed this Dec 13, 2021
@ncilfone
Copy link
Contributor

ncilfone commented Dec 13, 2021 •
edited
Loading

@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 [email protected]) 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)

@ncilfone ncilfone reopened this Dec 13, 2021
@ncilfone ncilfone merged commit b543443 into fidelity:dag_nesting Dec 13, 2021
ncilfone added a commit that referenced this pull request Dec 13, 2021
@ncilfone @gbmarc1
Final -- Improve handling of nested dependencies (#181)
ba4330c 
* fixes issues wrt more than 2 levels of class nesting references. was ssuming to fall back on defaults instead of recursing the config space to set the correct parameters.

* linted

* updated docs for cmd line overrides on nested classes

* fixed edge case from #152 where the default attrs object wasn't getting recursed to set config arg values within nested classes.

* adding test conf file

* wonky patch to deal with the monster under the bed that is #152. refactor should be thought about to handle all the parent/child relations in a cleaner manner

* WIP: Changing all the nesting deps to be handle via a DAG. Breaks most tests.

* fixes optional class refs.

* fix signature to deal with tuner -- should resolve all tests in /tune. Probably a bunch of vestigial code still to remove -- base tests still only 43/48

* fixing some tests (#163)

* fixing some tests

* some interesting work but breaking the tests

* test_command_line pass

* some cleaning and refactoring

* fix when optional nested value is None

* clearner refactor to handle type optionals

* "config" cannot be a general argument

* fix some more tests and windows path compatible

* s3 tests pass on windows

* all tests now are passing

* dont need the graph in builde space

* renamed everything to builder_space
and doc string

* some cleaning

Co-authored-by: mbelanger_explorance <[email protected]>

* Re-Improve handling of nested dependencies (#180)

* Nested values edge case fix (#160)

* fixes issues wrt more than 2 levels of class nesting references. was ssuming to fall back on defaults instead of recursing the config space to set the correct parameters.

* linted

* updated docs for cmd line overrides on nested classes

* fixed edge case from #152 where the default attrs object wasn't getting recursed to set config arg values within nested classes.

* adding test conf file

* fixing some tests

* some interesting work but breaking the tests

* test_command_line pass

* some cleaning and refactoring

* fix when optional nested value is None

* clearner refactor to handle type optionals

* "config" cannot be a general argument

* fix some more tests and windows path compatible

* s3 tests pass on windows

* all tests now are passing

* dont need the graph in builde space

* renamed everything to builder_space
and doc string

* some cleaning

* removing networkx dep

* linted

* removed dataclasses dep by swapping to a namedtuple

* fix-up of some tests. some were missing correct spock classes as *args. Some now raise a different exception with the new refactor

* moved help functionality to separate file for readability of the builder class

* documentation. almost finished

* finished doc strings. linted

* files for extra tests

* fixing issues with python3.6 which doesn't have the typing alias 'list' yet only 'typing.List'

* linted

Co-authored-by: mbelanger_explorance <[email protected]>

* updated readme

Co-authored-by: gbmarc1 <[email protected]>
Co-authored-by: mbelanger_explorance <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncilfone ncilfone Awaiting requested review from ncilfone

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gbmarc1 @ncilfone