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

Resolve dependencies refresh #2710

Closed
wants to merge 15 commits into from

Conversation

mikejhuang
Copy link
Contributor

@mikejhuang mikejhuang commented Aug 2, 2023

Include Python 3.11 compatibility, unpeg all dependencies and fix breaking changes from updated packages or downgrade packages.

Steps

  1. Use poetry to setup pyproject.toml file with python = ">=3.8,<3.12"
  2. Create a copy of requirements.txt without explicit versions at poetry_py38-311.txt
  3. poetry add `cat poetry_py38-311.txt`
  4. poetry does not resolve python version requirements and this needs to be adjusted manually
  5. xarray, scipy, and numpy versions adjusted to meet python version requirements
  6. poetry add `cat poetry_py38-311.txt` completed successfully and created solution of valid dependency ranges in pyproject.toml
  7. All dependencies are installed, poetry.lock file stores the actual installed versions for reproducibility
  8. Run tests by creating virtualenv with 3.8, 3.9, 3.10, 3.11
  9. For failing tests, either fix code or revert package version
    Final explicitly defined requirements

Fixes for compatibility

  1. pandas 1.1.5 -> 2.0.3
  2. numpy 1.23.5
    • np.bool, np.float, np.int are deprecated. Use 'bool', 'float', or 'int' instead.

Reverting versions instead of fixing

  1. hdmf=<3.4.7
  2. pandas<2.0.0
    • padas==2.0.3: VisualBehaviorProjectCache.get_ophys_session_table( index_column="ophys_experiment_id").index is type 'object', expected int64
      All the values in this index are int64 so I'm not sure why it was loaded as an object type.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Contributor

@morriscb morriscb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be trageted onto branch rc/2.16.0.

all_tried = False
while not all_tried:

matches_combination = np.ones((num_sweeps,),dtype=np.bool)
matches_combination = np.ones((num_sweeps,),dtype=bool)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You seem to input types besides bool as a string to dtype. Is there are a reason for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

float/int sizes (eg, float32 vs float64) can be automatically managed by defining it as a string. Bool does not have any variations.

setup.py Outdated
@@ -90,6 +90,8 @@ def prepend_find_packages(*roots):
"Programming Language :: Python :: 3.7",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to drop 3.7 correct?

Copy link
Contributor

@aamster aamster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see adoption of poetry fully fleshed out in this PR.

I see that you've made changes so that allensdk is compatible with python 3.11. That's good, but that could have been done without poetry.

We do not want to pin versions because this is used as a library, not an application. That was the original issue. You say that you've upinned dependencies but I don't see that in the PR. Looks like you've pinned dependencies to exact versions in requirements.txt

I do not understand why there are requirements defined in 3 files now. It should be defined only in pyproject.toml if we're going to use that

I am not going to adopt poetry right now for development and I don't see this automated in this PR or connected to the build process. I also don't really understand what problem poetry solves so it's hard for me to accept adoption of poetry into allensdk until I understand the benefits of it better.

@mikejhuang
Copy link
Contributor Author

I intended to consolidate the dependencies in pyproject.toml and remove the requirements.txt after I understood it better when reading about it more and discussing it with the rest of the team.

I renamed this PR from "Poetry dependency manager" to "Resolve dependencies refresh" since the Poetry aspect was a tool used to build the pyproject.toml but is not a requirement for this package. I believe it would be up to each developer to use Poetry if they believe it adds value to the way they manage dependencies. You can still manually edit the versions in the pyproject.toml file and use pip if you don't want Poetry to define them for you.

@aamster
Copy link
Contributor

aamster commented Aug 8, 2023

I believe it would be up to each developer to use Poetry if they believe it adds value to the way they manage dependencies.

I am not going to adopt poetry right now. That leaves Chris. @morriscb will you be adopting poetry for development? If the majority of developers are not going to adopt it, then let's remove pyproject.toml, poetry.lock, etc from this PR.

@morriscb
Copy link
Contributor

morriscb commented Aug 8, 2023

I believe it would be up to each developer to use Poetry if they believe it adds value to the way they manage dependencies.

I am not going to adopt poetry right now. That leaves Chris. @morriscb will you be adopting poetry for development? If the majority of developers are not going to adopt it, then let's remove pyproject.toml, poetry.lock, etc from this PR.

I do not plan to adopt poetry at this time, no.

@mikejhuang mikejhuang force-pushed the psb-172/resolve_dependency_refresh branch from 64cf2b8 to c1915de Compare August 9, 2023 21:58
@mikejhuang
Copy link
Contributor Author

This should be trageted onto branch rc/2.16.0.

I recall agreeing that it should be targeted to main to expedite this so that users of SWDB would have these changes.

@morriscb
Copy link
Contributor

The fix for SWDB has already been deployed and is working. That was the tickets we were referring to, not tihs one.

@mikejhuang
Copy link
Contributor Author

The fix for SWDB has already been deployed and is working. That was the tickets we were referring to, not tihs one.

I recall it was also for this one since it would reduce the compatibility issues users might run into during that workshop.

@mikejhuang mikejhuang changed the base branch from master to rc/2.16.0 August 10, 2023 18:44
@mikejhuang mikejhuang changed the base branch from rc/2.16.0 to master August 10, 2023 19:21
@mikejhuang mikejhuang force-pushed the psb-172/resolve_dependency_refresh branch from 4072574 to 86b8c57 Compare August 10, 2023 19:23
@mikejhuang mikejhuang closed this Aug 10, 2023
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