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.

Sign up for GitHub

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

Jump to bottom

Upgrade SQLAlchemy to v2 #1225

Open
4 of 13 tasks
kalbfled opened this issue Apr 19, 2023 · 17 comments · May be fixed by #1684
Open
4 of 13 tasks

Upgrade SQLAlchemy to v2 #1225

kalbfled opened this issue Apr 19, 2023 · 17 comments · May be fixed by #1684
Assignees
@mchlwellman @kalbfled @MackHalliday
Labels
dependencies Pull requests that update a dependency file Dev Reviewed Notify Off-track PM Reviewed QA Reviewed QA

Comments

@kalbfled
Copy link
Member

kalbfled commented Apr 19, 2023 •
edited by mchlwellman
Loading

User Story - Business Need

Upgrade SQLAlchemy from v1.4.x to the latest 2.x version.

User Story(ies)

As a VA Notify developer
I want to upgrade SQLAlchemy
So that we minimize vulnerabilities and future upgrade pains.

  • discuss w/ QA

Additional Info and Resources

Engineering Checklist

  • Review relevant upgrade/migration docs before getting started
  • Upgrade the effected packages in requirements-app.txt
  • Build a new Docker image
  • Run the app locally. Observe failures. Fix them.
  • Deploying the app may produce different results than local runs, so deploy to dev and observe/fix failures.
    • Run Regression
  • Update requirements.txt as described in README.md

Acceptance Criteria

  • The image builds
  • All unit tests pass with the latest versions of the effected packages

QA Considerations

  • QA regression passes
  • Pay extra care to JSON responses
  • Performance Testing to ensure there are no significant slowdowns to investigate

Potential Dependencies

Leave blank if n/a

Out of Scope

Do not upgrade Celery, marshmallow, or marshmallow-sqlalchemy. Separate tickets exist for those upgrades.
@kalbfled kalbfled added dependencies Pull requests that update a dependency file Notify labels Apr 19, 2023
@kalbfled kalbfled mentioned this issue Apr 19, 2023
Closed
11 tasks
@mjones-oddball
Copy link

Hey team! Please add your planning poker estimate with Zenhub @cris-oddball @justaskdavidb2 @k-macmillan @kalbfled @ldraney @nikolai-efimov

@cris-oddball cris-oddball mentioned this issue Aug 9, 2023
Closed
10 tasks
@cris-oddball cris-oddball changed the title Upgrade SQLAlchemy Upgrade SQLAlchemy to v2 Aug 9, 2023
@mjones-oddball mjones-oddball mentioned this issue Nov 15, 2023
Closed
5 tasks
@mjones-oddball
Copy link

Please add your planning poker estimate with Zenhub @edDocMe360

@mjones-oddball
Copy link

Please add your planning poker estimate with Zenhub @EvanParish

@ldraney
Copy link

ldraney commented Nov 27, 2023

Dave brought up that there may be implications here when moving to AWS.

@mjones-oddball
Copy link

Was an 8. Going to break out query updates and repoint when ready.

@kalbfled
Copy link
Member Author

@k-macmillan While revising this and creating #1648, I removed your legacy syntax "straggler" because there are actually more than one still on app/. #1648 should remove all of them and provides a grep statement to find them.

@EvanParish EvanParish self-assigned this Mar 4, 2024
@EvanParish
Copy link

After meeting with Kyle yesterday and discussing this, there's a little more complexity than we had hoped. I'm still working on getting the app running locally. It looks like I might have to manually define the history tables.

@EvanParish EvanParish linked a pull request Mar 6, 2024 that will close this issue
Draft
21 tasks
@EvanParish EvanParish linked a pull request Mar 6, 2024 that will close this issue
#1225 Upgrade SQLAlchemy to v2 #1684
Draft
21 tasks
@EvanParish
Copy link

I created a PR for the progress so far and will have a discussion with @mchlwellman when he's ready to pick up this ticket.

@mchlwellman
Copy link

Just getting going on this, I have spoken with Evan. I'll have more of an update tomorrow once I look more in depth.

@mchlwellman
Copy link

I am still resolving errors, the current one that is taking some time is this -

Traceback (most recent call last):
  File "/Users/michaelwellman/.pyenv/versions/va-notify/bin/flask", line 8, in <module>
    sys.exit(main())
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/flask/cli.py", line 1107, in main
    cli.main()
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/click/core.py", line 1688, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/click/decorators.py", line 33, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/flask/cli.py", line 385, in decorator
    app = ctx.ensure_object(ScriptInfo).load_app()
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/flask/cli.py", line 337, in load_app
    app = locate_app(import_name, name)
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/flask/cli.py", line 247, in locate_app
    __import__(module_name)
  File "/Users/michaelwellman/dev/va/notification-api/application.py", line 31, in <module>
    create_app(application)
  File "/Users/michaelwellman/dev/va/notification-api/app/__init__.py", line 221, in create_app
    register_blueprint(application)
  File "/Users/michaelwellman/dev/va/notification-api/app/__init__.py", line 238, in register_blueprint
    from app.service.rest import service_blueprint
  File "/Users/michaelwellman/dev/va/notification-api/app/service/rest.py", line 73, in <module>
    from app.schemas import (
  File "/Users/michaelwellman/dev/va/notification-api/app/schemas.py", line 127, in <module>
    class UserSchema(BaseSchema):
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/marshmallow/schema.py", line 114, in __new__
    klass._declared_fields = mcs.get_declared_fields(
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/marshmallow_sqlalchemy/schema/sqlalchemy_schema.py", line 91, in get_declared_fields
    fields.update(mcs.get_declared_sqla_fields(fields, converter, opts, dict_cls))
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/marshmallow_sqlalchemy/schema/sqlalchemy_schema.py", line 130, in get_declared_sqla_fields
    converter.fields_for_model(
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/marshmallow_sqlalchemy/convert.py", line 111, in fields_for_model
    field = base_fields.get(key) or self.property2field(prop)
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/marshmallow_sqlalchemy/convert.py", line 146, in property2field
    field_class = field_class or self._get_field_class_for_property(prop)
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/marshmallow_sqlalchemy/convert.py", line 210, in _get_field_class_for_property
    column = prop.columns[0]
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/sqlalchemy/util/langhelpers.py", line 1327, in __getattr__
    return self._fallback_getattr(key)
  File "/Users/michaelwellman/.pyenv/versions/3.10.13/envs/va-notify/lib/python3.10/site-packages/sqlalchemy/util/langhelpers.py", line 1296, in _fallback_getattr
    raise AttributeError(key)
AttributeError: columns

@kalbfled kalbfled self-assigned this Mar 11, 2024
@kalbfled
Copy link
Member Author

kalbfled commented Mar 11, 2024 •
edited
Loading

I'm picking up this ticket because Michael is out today.

@mjones-oddball mjones-oddball mentioned this issue Mar 13, 2024
Closed
9 tasks
@mchlwellman
Copy link

mchlwellman commented Mar 14, 2024 •
edited
Loading

I am using these versions of libs -

app-1  | Flask                                        3.0.2
app-1  | Flask-Bcrypt                                 1.0.1
app-1  | Flask-Cors                                   4.0.0
app-1  | Flask-JWT-Extended                           4.6.0
app-1  | flask-marshmallow                            1.2.0
app-1  | Flask-Migrate                                4.0.7
app-1  | Flask-SQLAlchemy                             3.1.1
app-1  | flask-marshmallow                            1.2.0
app-1  | marshmallow                                  3.21.1
app-1  | marshmallow-sqlalchemy                       1.0.0
app-1  | Flask-SQLAlchemy                             3.1.1
app-1  | marshmallow-sqlalchemy                       1.0.0
app-1  | SQLAlchemy                                   2.0.28

This error is still occurring -
sqlalchemy.exc.InvalidRequestError: When initializing mapper Mapper[User(users)], expression 'user_to_organisation' failed to locate a name ("name 'user_to_organisation' is not defined"). If this is a class name, consider adding this relationship() to the <class 'app.model.user.User'> class after both dependent classes have been defined.

@kalbfled
Copy link
Member Author

I upgraded SQLAlchemy to v2 and upgrade related packages to their newest versions. I have been converting model declarations to the SQLAlchemy v2 "Declarative" syntax and removing inputs to schemas' "exclude" attribute in app/schema.py. I should know some time Monday if this is going to get the app in a runnable condition.

kalbfled added a commit that referenced this issue Mar 18, 2024
@kalbfled
#1225 Upgrade SQLAlchemy to v2.0, and also upgrade supporting depende…
8c79108 
…ncies.
@kalbfled kalbfled mentioned this issue Mar 18, 2024
Draft
20 tasks
@kalbfled
Copy link
Member Author

I managed to run the app on my branch (draft PR), but there are many errors:

1523 failed, 1270 passed, 16 skipped, 79 xfailed, 3 xpassed, 1528 errors

This branch does not include the recently merged Poetry changes. I don't want to mess with that until all the tests pass.

I suspect that I need to continue updating model definitions in models.py to use the newer syntax.

@kalbfled
Copy link
Member Author

kalbfled commented Mar 18, 2024 •
edited
Loading

I updated models.py to the newer declarative format, but that didn't change the test results. Error messages suggest that I need to update how models relationships (foreign keys, etc.) are defined. I'm going to try to troubleshoot by running a specific test

@kalbfled
Copy link
Member Author

After discussion with @k-macmillan and @mchlwellman, we have decided to pause work on this ticket and #1224, which is part of the same task. I do not have any new tests passing compared to what I posted yesterday. I began updating relationships in app/models.py and app/models/user.py because I was seeing many test errors related to foreign keys, but that work is tedious because doing it right requires knowing the intent behind the code. Is the relationship 1-to-many? Many-to-many? Etc.? Once the intent is understood by exploring how the relationship attributes are actually used in the code, we can update the syntax using the appropriate declarative relationship definition.

We discussed that completing the tickets that remove unused tables before returning to this ticket might be the best course of action.

A draft PR exists with the work Michael and I have completed thus far. I have not incorporated the Poetry implementation yet.

Below are the big picture steps I took to get the app and unit tests to run. See the PR for details. All steps other than altering model/table definitions are necessary to get the application to run locally or, once running, to run the tests. Michael pushed changes to upgrade migration code, which also needs to change to use SQLAlchemy v2.

  1. Upgrade SQLAchemy, flask-sqlachemy, marshmallow, flask-marshmallow, and marshmallow-sqlalchemy to the newest versions. Without updating at least some of the supporting packages in addition to SQLAlchemy, the app won't start. It's easiest to do all these at once. Rebuild the application and test images.
  2. Make all changes as described in the flask-sqlalchemy v3.1 quick start guide. In particular, note the "Initialize the Extension" section. All read the Models and Tables page.
  3. Using the PR, view the changes to app/history_meta.py. I commented out code to clear an error that prevents the app from running. This is not a solution, and I expect any test involving model histories to fail as a consequence.
  4. In app/schemas.py, I removed many elements from schemas' "exclude" meta-attribute because I was getting errors about the model not containing those fields when I tried running the app. At a glance, the offending fields either aren't defined on the model or are relationship fields. Once relationships are properly defined, restoring those fields to the "exclude" list might be appropriate. If this is the case, I expect tests of GET responses to fail.
  5. In tests/conftest.py, see the changes I made for "reflection" and correctly to access table metadata. These are breaking syntax changes when upgrading to SQLAlchemy v2.

kalbfled added a commit that referenced this issue Mar 19, 2024
@kalbfled
Upgrade SQLAlchemy to v2.0, and also upgrade supporting dependencies. #…
5c188ad 
…1224 #1225

Co-authored-by: Michael Wellman <michaelwellman@MichaelacStudio.lan>
@npmartin-oddball
Copy link

Leaving for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mchlwellman mchlwellman

@kalbfled kalbfled

@MackHalliday MackHalliday

Labels
dependencies Pull requests that update a dependency file Dev Reviewed Notify Off-track PM Reviewed QA Reviewed QA
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

#1225 Upgrade SQLAlchemy to v2 department-of-veterans-affairs/notification-api
9 participants
@mchlwellman @kalbfled @MackHalliday @k-macmillan @mjones-oddball @EvanParish @ldraney @cris-oddball @npmartin-oddball