updating environment yaml to include bokeh

[taylorgibson]

[ ] Wrote test for feature
[ ] Added changes to CHANGELOG.md

Changes proposed:
Updating the linux_environment.yml and osx_environment.yml so directions in DEVELOPER.md result in make test passing all tests. Closes #579

[coveralls]

Coverage: 92.699%. Remained the same when pulling 827d7bc on taylorgibson:issue579 into 5c814c7 on data-8:master.

[adnanhemani]

Hi, the Github Action test suite passes without this change and that is also reading/installing from the linux_environment.yml file (from here: https://github.com/data-8/datascience/blob/master/.github/workflows/run_tests.yml#L18). So it seems like this might not be the true reason why tests aren't passing on your machine. Could you please paste a full log with all the tests that are failing for you and I can try to take a look at what might be going on?

[taylorgibson]

Hi @adnanhemani

It looks like the Github Action (https://github.com/data-8/datascience/blob/master/.github/workflows/run_tests.yml#L18) is doing a bit more than the directions found in DEVELOPERS.md before tests are run which leads to the test suite passing during the run of the action. In particular, line 36 is installing another set of requirements into the environment from requirements-tests.txt, which include bokeh.

make test is not called in the Github Action until after these additional requirements are installed, which would explain why they pass when the Github Action runs but do not pass when following the instructions found in DEVELOPERS.md locally. The DEVELOPERS.md directions do not indicate that installing requirements from requirements-tests.txt is necessary, nor does it seem that make install or make test ensures that these requirements are installed in some other way. You can confirm in a recent run of the Github Action that bokeh is only attempted to be installed from requirements-tests.txt here: https://github.com/data-8/datascience/actions/runs/4072809364/jobs/7015985102#step:5:118,

It looks like bokeh is only required to import a set of sample data for the subsequent tests to use. The tests do not use bokeh in any other way. Any errors following the first test are simply because the tests do not have access the required dataframe, due to the failure of the first test. I've attached a full capture of my terminal output if that helps sort things out datascience-bugtest.txt.

I dug in a bit to the other code in the repository, and it looks like setup.py includes the fact that bokeh is required for the tests in the list tests_requires on line 12, but it's unclear to me that these additional requirements are installed when running make install / setup.py or make test / tests.py.

It seems to me that one of the following changes needs to occur:

• bokeh should be added to the environment yaml files, as done in this PR
• directions in DEVELOPERS.md be changed to indicate packages in requirements-tests.txt should be installed into the environment.
• setup.py and/or tests.py be updated so that the required packages for testing are installed during make install or make test to ensure tests pass.

I would defer to the maintainers of the repository as to which path is preferred and would work well with the broader goals of developing this tool.

[adnanhemani]

Hi @taylorgibson,

Thank you for the in-depth analysis here - it is very well appreciated and everything you've mentioned in spot-on in accuracy. As per the options you've presented to solve this problem, I think adding the manual step for installing requirements-tests.txt n the markdown file is the right way forward - here is my analysis:

• You're correct that Bokeh is not required for the library functionality - and I'd say it shouldn't. datascience is meant to be a lightweight library (as much as possible), so any dependency we make in the actual library needs to be vital for the core functionality of the library as a whole. As a result, adding it to the environment YAML files would be the wrong choice due to the library's overall goals and design choices.
• The DEVELOPERS.md file has not gone through a serious review in quite some time. The last change to the file was to remove a lot of old processes that we no longer follow. Perhaps this would be a good time to do that review.
• Installing static libraries during a make call is generally frowned upon.

With all of this said, I think the best way forward would be to add these instructions to install the test requirements to the DEVELOPERS.md file. If you are able to (and would like to) revamp that file, I'd be more than glad to review those changes! I'll close this PR for now - if @davidwagner has an opposing opinion, I'll let him re-open the ticket and comment why.

Thanks for all your work!

[taylorgibson]

Thanks @adnanhemani for the additional information.

I noticed this morning that running tests through tox works fine, even though tox doesn't install bokeh into the conda environment. I thought that was curious, so I looked at how the tests are run differently when using tox instead of make test.

I noticed that tox.ini calls python setup.py test to initiate pytest while make test runs python tests.py. Both methods attempt to use the pytest.main(...) command to run the test suite, however using python setup.py test leverages setuptools to include any dependencies that are only required for testing. It does so by specifying dependencies in the list test_requires. I think for simplicity and consistency both tox and make test should run pytest the same way, and since python setup.py tests seems to currently do the job, my thought was that we should go that route. It would be a simple change to the test target in the Makefile.

However, I will note that using setuptools to invoke pytest seems to be deprecated: https://docs.pytest.org/en/7.1.x/explanation/goodpractices.html#do-not-run-via-setuptools. So, while moving to python setup.py test in the Makefile may be a good short term solution, it seems like moving completely to tox for facilitating testing would be a better long term strategy.

So, let me know what you think the best course of action would be:

• Keep the status quo: make the changes as described above in the test target in the project Makefile to run python setup.py test. This would be quick, easy, and wouldn't require a change in documentation. It would ✨ just work ✨ (in theory).
• Move to tox: move testing fully to depend on tox and move away from the deprecated python setup.py test workflow. This would require a little trial and error to ensure the workflow still works as intended and avoids deprecated commands. I've tried it out a bit locally and it seems fairly straightforward.
• Something else: is there another preferred way to handle this? I realize that this issue is likely not a priority since it doesn't impact usability of the library for users, so if it's best to just leave this alone please let me know!

I'll wait to hear from you before making any additional changes / submitting a PR.

[adnanhemani]

Hi @taylorgibson, great observation. So the history behind tox support is that someone contributed it to the project so we accepted it, but I don't think any of the maintainers are using it when we're developing (side note, we also use make test in the Github Action for CI/CD - unsure if tox will cause any issues there. I had a painstaking experience shifting our CI/CD pipelines over and would really prefer not to mess with the pipeline as long as it's working as expected 😆)

And we did have python setup.py test previously as the make target for make test but purposefully changed that based on the deprecation that you've mentioned. If you have the time, we can try two things:

1. Try to move everything over to tox (bulletpoint 2 in your latest suggestions). We can then hook up the make test target to just call tox in the background. This might be the cleanest way to approach this problem, but we will need to ensure that unit tests and code coverage aren't affected by the changes (we can check this using GitHub Actions and Coveralls)
2. In tests.py, we can try to install the testing libraries before we run PyTest. I don't think this is a very clean solution, but at this point, if it works, it works - clearly something is broken today with the way how things are right now.
3. If both of these fail, I think our best bet might be to leave it alone and put it as a required step in the DEVELOPERS.md file, as suggested before.

Let me know if you have any other thoughts or questions - thank you for all this investigative effort!

[taylorgibson]

See new PR #583