Switch build from setuptools to Poetry and move tests path

[kenibrewer]

Description

This pull request implements #278 and switches the project build approach from setuptools to pyproject.toml using poetry for dependency management.

Additional changes made during conversion:

• tests directory moved from pycytominer/tests to <root>/tests
• Refactored tests/test_cyto_utils/test_DeepProfiler_processing.py to move process heavy parts into test_functions
• Changed workflows to use poetry for test / building / publishing
• Removed .vscode from gitignore and added pytest, pyenv, and poetry configuration relevant to devcontainers
• Added poetry precommit hooks to keep the poetry.lock up to date and requirements{-dev}.txt in sync with pyproject.toml
• Change cyto_utils.collate.py to use python functions instead of cli commands.
• Documentation update to CONTRIBUTING.md about poetry

What is the nature of your change?

• Bug fix (fixes an issue).
• Enhancement (adds functionality).
• This change requires a documentation update.

Checklist

Please ensure that all boxes are checked before indicating that a pull request is ready for review.

• I have read the CONTRIBUTING.md guidelines.
• My code follows the style guidelines of this project.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• I have made corresponding changes to the documentation.
• My changes generate no new warnings.
• New and existing unit tests pass locally with my changes.
• I have added tests that prove my fix is effective or that my feature works.
• I have deleted all non-relevant text in this pull request template.

[codecov-commenter]

Codecov Report

Merging #303 (a2a413d) into master (c4de0a9) will increase coverage by 0.06%.
The diff coverage is 96.53%.

@@            Coverage Diff             @@
##           master     #303      +/-   ##
==========================================
+ Coverage   95.34%   95.40%   +0.06%     
==========================================
  Files          57       56       -1     
  Lines        3134     3134              
==========================================
+ Hits         2988     2990       +2     
+ Misses        146      144       -2     

Flag	Coverage Δ
unittests	95.40% <96.53%> (+0.06%)	⬆️

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

Files Changed	Coverage Δ
pycytominer/cyto_utils/cell_locations.py	84.21% <ø> (ø)
tests/test_aggregate.py	98.80% <ø> (ø)
tests/test_annotate.py	100.00% <ø> (ø)
tests/test_consensus.py	100.00% <ø> (ø)
tests/test_cyto_utils/conftest.py	100.00% <ø> (ø)
tests/test_cyto_utils/test_annotate_custom.py	98.46% <ø> (ø)
tests/test_cyto_utils/test_cell_locations.py	100.00% <ø> (ø)
tests/test_cyto_utils/test_cells.py	96.44% <ø> (ø)
tests/test_cyto_utils/test_cp_image_features.py	100.00% <ø> (ø)
tests/test_cyto_utils/test_feature_drop_outlier.py	100.00% <ø> (ø)
... and 19 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[kenibrewer]

@gwaybio What are your thoughts about switching from master to the more inclusive main default branch for the project as a part of this PR?

[gwaybio]

I am in favor @kenibrewer , please make the switch if it's straightforward. Thanks!

[d33bs]

Nice work! Excited to see the transition into a pyproject.toml file and Poetry. I left a few comments regarding the changes and also wanted to follow up here with additional questions. Generally I'm requesting changes due to these more wide-reaching questions and hoping to better understand the development story behind your design considerations.

• Given how different Poetry and pyproject.toml development configuration can be, I highly recommend making documentation additions to the CONTRIBUTING.md content or perhaps in the Sphinx docs. This would help support the current and future developer community surrounding Pycytominer. Without this and the current state of retaining the requirements*.txt means developers may misinterpret how environment management is handled.
• Regarding the changes to tests/test_cyto_utils/test_DeepProfiler_processing.py (and perhaps others): were these changes necessary in order to maintain passing tests in transitioning to the new setup? I appreciate the work towards changes in those files. At the same I felt that those changes might be different enough to become their own PR's to help distinguish the changes going on with the setup process. Reducing the number of additional changes alongside setup and environment management would decrease the chance for unintended or complex consequences and increase the overall assurance that the setup works like it did prior to the changes in this PR. Just the same, I'm seeking to understand and learn from you - so open to your thoughts and interested to hear more!

[kenibrewer]

• Given how different Poetry and pyproject.toml development configuration can be, I highly recommend making documentation additions to the CONTRIBUTING.md content or perhaps in the Sphinx docs. This would help support the current and future developer community surrounding Pycytominer. Without this and the current state of retaining the requirements*.txt means developers may misinterpret how environment management is handled.

I agree. I added an additional todo item to this PR for documentation that I left unchecked. I wanted to get feedback on the initial set of changes that I proposed before doing the documentation changes.

• Regarding the changes to tests/test_cyto_utils/test_DeepProfiler_processing.py (and perhaps others): were these changes necessary in order to maintain passing tests in transitioning to the new setup? I appreciate the work towards changes in those files. At the same I felt that those changes might be different enough to become their own PR's to help distinguish the changes going on with the setup process.

The changes to the other test files were necessary as a part of changing their paths. test_DeepProfiler_processing.py required some path changes too, but I ended up doing more work on that file because it took me a several hours to realize the root cause of #304. Having pytest crash during test collection with no informative error message was a very frustrating experience. Without these changes, I am unable to run tests locally in a Github Codespace and can only discover failed tests when I run the github pipelines.

In general I agree with the strategy of trying to limit the "bast radius" of PRs. However, in the case of this PR with changes to core aspects of the code is built I think it is worth being a bit more flexible in allowing a broader set of related changes required to allow the program to function as a whole.

[kenibrewer]

@d33bs I still have some outstanding changes related to versioning and documentation, but it would be good to get another review from you at this point for the changes so far.

[d33bs]

Nice work @kenibrewer ! I left a couple comments about the additional changes added. I'm marking this as requesting changes due to what I felt like might be an unclosed database connection (which could result in unexpected behavior) - otherwise things looked good to me. Given the changes to collate.py I recommend including those details upfront in the PR title or description to help observe and reason through what historically changed in this PR.

[kenibrewer]

@d33bs @gwaybio This PR is ready to merge.

[gwaybio]

Thanks for this contribution @kenibrewer !

I have one question regarding the collate changes. Is it still possible to use collate from the command line?

We have collaborators who use collate this way, and if we update collate usage, then they will not benefit from future pycytominer improvements. Is there a way to keep the collate CLI for now? FWIW, we believe CytoTable will supercede collate in the very near term, so we can stop supporting collate in future pycytominer versions.

[kenibrewer]

I have one question regarding the collate changes. Is it still possible to use collate from the command line?

@gwaybio Nothing about the changes to collate.py make collate_cmd.py unable to be used from the command line. Collate simply invokes fewer command line programs via subprocess.run and uses python functions instead when it gets invoked. All changes to collate were internal in nature and there are no changes to usage of the respective tools.

[kenibrewer]

Perhaps I should clarify. The problem the collate.py changes were meant to address was only encountered when using the vscode pytest integration to run tests. I believe the root cause is that vscode doesn't fully initialize the poetry virtual environment. During this special vscode pytest, all the python modules are properly available via imports, but external command line tools are not added to the path. This causes issues when the python code executes a subprocess.run() command expecting to find an external cli that is only present in the poetry venv.

I also went ahead and double checked that all the CLI tools that vscode pytest couldn't find, were actually properly installed and available in the PATH when the poetry venv was properly initialized from the command line or when the built package was installed via pip.

[gwaybio]

Thanks for this explanation @kenibrewer ! And thank you for this important contribution :)

@d33bs - please feel free to merge when you are ready.

[d33bs]

Thank you so much @kenibrewer for the tremendous contributions in this PR! I feel good about how things look and would like to merge.

An aside: in the (I hope) kindest possible way I wanted to offer a suggestion for future work here or elsewhere (I have no qualms as is and these comments are intended as continuous improvement). There were a number of ideas in this PR which formed a larger and less modular group of code "chunks" to review (and historically understand). From a development standpoint, this is completely expected; when working on one thing we often can see other related things that need to get fixed.

When a "big enough" (totally subjective to the project and developer community) related but somewhat isolated chunk of work like this comes up it can be a good idea to spur a "fix / related" branch and PR, updating the original feature branch after the "fix" is merged to help keep things modular and isolated. While this can take longer, it can help isolate the changes for the developer(s), reviewers, and those looking at the code in the future to understand why things changed. It also might effect how other in-flight PR's and feature branches grow. When we isolate the discussion around singular and smaller chunks of code this way we offer ourselves a chance to be more careful, deliberate, and descriptive about the changes taking place.

A visual of how this might look would be the following:

    %%{init: { 'logLevel': 'debug', 'theme': 'default' , 'themeVariables': {
      'git0': '#6366F1',
      'git1': '#10B981',
      'git2': '#F59E0B',
      'gitBranchLabel1': '#ffffff',
      'gitBranchLabel2': '#ffffff'
} } }%%
    gitGraph
       commit id: "..."
       commit id: "original state"
       branch feature
       checkout feature
       commit id: "discover new needs"
       checkout main
       branch related_fix
       checkout related_fix
       commit id: "fix those needs"
       checkout main
       merge related_fix id: "merged fix (main)"
       checkout feature
       merge main id: "merged fix (feature)"
       checkout main
       merge feature id: "merge feature"

Loading

Only offering this up as a suggestion for future consideration. Thank you again so much for your thoughtful and thorough contributions to Pycytominer!

[kenibrewer]

When a "big enough" (totally subjective to the project and developer community) related but somewhat isolated chunk of work like this comes up it can be a good idea to spur a "fix / related" branch and PR, updating the original feature branch after the "fix" is merged to help keep things modular and isolated.

Thanks for this suggestion! This is definitely one of the more complicated software PR's I've worked on, so it is helpful to know about the best practices I can apply for future work.