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

Remove fastapi-utls #17202

Closed
wants to merge 51 commits into from
Closed

Conversation

jdavcs
Copy link
Member

@jdavcs jdavcs commented Dec 17, 2023

Ref #17185, #12541

This removes the dependency on fastapi-utils that is a blocker for SQLAlchemy 2.0. See #17185 for rationale. There has also been a discussion on the backend channel on Dec 15 (ref)

To simplify the review of this PR (re: discussion on the channel), I've split it into the following commits:

  • One commit per module to remove the class decorator, move the class attribute(s) to the methods, and remove the self parameter from the method definition (thus making the methods static). There is almost no indentation change here, which, I hope, will simplify reviewing.
  • One final commit that reformats the updated modules by ONLY removing the class definition, thereby converting the static methods into standalone functions. (Indentation is changed here, so everything is marked as red)

A few exceptions:

  • In a few cases there were helper methods (prefixed with an underscore) that were called from the endpoint-handling methods. I moved those our of the class and passed the resolved dependencies to them directly (as it should have been done in the first place, and indeed was already done in some cases)
  • There are a few final commits that remove the cbv definition and update the dependencies.
  • item_tags still has a class. However, here we have 3 classes dynamically generated, and keeping the endpoint-handling methods as static methods works just as fine.

So, overall, the process went as follows:

  1. Pick a module that uses cbv
  2. Remove the decorator
  3. Move the dependency defined on the class to each endpoint-handling method; remove the self parameter from each method.
  4. Change the call to the dependency inside the method (remove self.)
  5. If there were any helper methods, move them out of the class, remove the self parameter, and pass the dependency to them.
  6. Repeat while grep cbv is not empty.
  7. Replace classes with standalone functions
  8. Update dependencies.

I hope this helps.

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@github-actions github-actions bot added this to the 23.2 milestone Dec 17, 2023
@jdavcs jdavcs requested a review from a team December 17, 2023 21:17
@jdavcs jdavcs added kind/refactoring cleanup or refactoring of existing code, no functional changes and removed area/tool-framework area/libraries Data libraries area/packaging labels Dec 17, 2023
@@ -2,16 +2,16 @@ aiohttp==3.9.1 ; python_version >= "3.8" and python_version < "3.12"
aiosignal==1.3.1 ; python_version >= "3.8" and python_version < "3.12"
alabaster==0.7.13 ; python_version >= "3.8" and python_version < "3.12"
amqp==5.2.0 ; python_version >= "3.8" and python_version < "3.12"
anyio==4.1.0 ; python_version >= "3.8" and python_version < "3.12"
Copy link
Member

Choose a reason for hiding this comment

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

I'd only remove fastapi-utils without all the unrelated dependencies updates.

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely! I think I ran the update script, but I don't have to. I'll fix this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@jdavcs jdavcs force-pushed the dev_remove_fastapi_utils_2 branch from edf6542 to 1220475 Compare December 17, 2023 23:05
Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

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

I don't think this is heading the right direction, as discussed extensively on the backend channel.

@jdavcs
Copy link
Member Author

jdavcs commented Dec 18, 2023

I don't think this is heading the right direction, as discussed extensively on the backend channel.

I've provided a summary of my point in the accompanying issue.

@jdavcs jdavcs mentioned this pull request Dec 18, 2023
4 tasks
@jdavcs
Copy link
Member Author

jdavcs commented Dec 18, 2023

Superseded by #17205

@jdavcs jdavcs closed this Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/API area/dependencies kind/refactoring cleanup or refactoring of existing code, no functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants