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] Safe Upgrade to Modern TensorFlow 2.x #247

Closed
wants to merge 3 commits into from
Closed

[WIP] Safe Upgrade to Modern TensorFlow 2.x #247

wants to merge 3 commits into from

Conversation

arose13
Copy link
Contributor

@arose13 arose13 commented Apr 9, 2020

By the way, is it really the case that there are no tests in this project for the neural network components or am I missing something

@arose13
Copy link
Contributor Author

arose13 commented Apr 9, 2020

Nevermind I found the tests. I guess you guys have a good reason why the test are located inside the econml rather than a test folder.

@kbattocchi
Copy link
Collaborator

kbattocchi commented Apr 9, 2020

Nevermind I found the tests. I guess you guys have a good reason why the test are located inside the econml rather than a test folder.

Not sure about "good" reason, but it's one of the two major ways people organize python tests (see, e.g. https://docs.pytest.org/en/latest/goodpractices.html#tests-as-part-of-application-code). I can't remember if we had a specific reason to choose this rather than putting the tests beside the package when starting the project, but it has worked fine for us so far.

By the way, please cherry pick 0f5ddfe 23a2d6b into your branch so that the automated tests can run - Azure Pipelines has deprecated one of VM images that we were using before, which is why the checks are failing immediately.

@msftclas
Copy link

msftclas commented Apr 21, 2020

CLA assistant check
All CLA requirements met.

@kbattocchi
Copy link
Collaborator

I apologize for leaving this hanging - it turns out that many of our other dependencies were also pretty limited (e.g. we had capped sklearn < 0.22 and didn't support Python 3.8 at all), so I incorporated loosening our tensorflow restriction into that set of changes, which has now been merged (as #210). We hope to get a new release out with these changes as well as a few other PRs within the next few weeks. Thanks for suggesting the change and contributing the PR, but I'm going to close it now since the other one supersedes it.

@kbattocchi kbattocchi closed this Aug 11, 2020
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.

3 participants