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

Upgrade SQLAlchemy to 2.0.22 and Pandas to 1.5.3 #2572

Merged
merged 37 commits into from
Nov 1, 2023
Merged

Conversation

IsaacMilarky
Copy link
Contributor

@IsaacMilarky IsaacMilarky commented Oct 30, 2023

Description

  • Update SQLAlchemy to version 2.0.22
  • Update Pandas to version 1.5.3
  • Removed redundant ORM Relationship definitions
  • Fix MacOS not finding location of go
  • Fix Alembic env.py not working
  • Fix broken util endpoints

This PR fixes #2557 #2556

Signed commits

  • Yes, I signed my commits.

Ulincsys and others added 30 commits July 3, 2023 18:47
- Switch to SQLAlchemy ORM syntax for repo filtering
    - Will replace the large SQL string in repo_load_controller.py
    - Current implementation located in augur/api/view/utils.py
- Add clearer error message when page number is incorrect on repo view
- Upgrade SQLAlchemy to v2.0.17
    - Required for new syntax that the old version does not support
        - Specifically `ilike` and `regexp_replace`

Signed-off-by: Ulincsys <[email protected]>
Signed-off-by: Sean P. Goggins <[email protected]>
Signed-off-by: Sean P. Goggins <[email protected]>
Signed-off-by: Sean P. Goggins <[email protected]>
… with connection object. This allows a commit to be implicit within the resulting context manager and emulates the functionality of the now removed autocommit

Signed-off-by: Isaac Milarsky <[email protected]>
…nsert_data method. fix missing .begin() call in block to deal with return columns. fix dict conversion for list of rows in sqlalchemy 2.x in insert_data and contributor breadth worker

Signed-off-by: Isaac Milarsky <[email protected]>
Signed-off-by: Isaac Milarsky <[email protected]>
Signed-off-by: Isaac Milarsky <[email protected]>
Updating SQLAlchemy_upgrade branch from dev
Signed-off-by: Isaac Milarsky <[email protected]>
Signed-off-by: Isaac Milarsky <[email protected]>
cache_files_requested.append(request)
return render_template('index.j2', body="loading", title="Loading", d=dest, query_key=query, api_url=getSetting('serving'))

with DatabaseEngine() as engine:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to merge for testing, but before this makes it into main, we need to ensure that the repo load controller gets updated with the below syntax (or something similar). This is just here for testing, and is not intended to be final

Signed-off-by: Isaac Milarsky <[email protected]>
@sgoggins sgoggins requested review from Ulincsys and ABrain7710 and removed request for sgoggins October 31, 2023 13:05
Copy link
Contributor

@ABrain7710 ABrain7710 left a comment

Choose a reason for hiding this comment

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

I think all the engine.connect() should be using a context manager to ensure the connection is being disposed of. I also don't think we should be removing valid relationships from the orm as this removes functionality without any benefit that I can see

augur/api/metrics/commit.py Outdated Show resolved Hide resolved
augur/application/db/models/augur_operations.py Outdated Show resolved Hide resolved
augur/tasks/data_analysis/insight_worker/tasks.py Outdated Show resolved Hide resolved
Signed-off-by: Isaac Milarsky <[email protected]>
Signed-off-by: Isaac Milarsky <[email protected]>
@IsaacMilarky
Copy link
Contributor Author

I think all the engine.connect() should be using a context manager to ensure the connection is being disposed of. I also don't think we should be removing valid relationships from the orm as this removes functionality without any benefit that I can see

I made the suggested changes. I think the orm relationship thing should be resolved in a separate pr so I just reverted all of my changes to the orm relationships for now.

Copy link
Contributor

@ABrain7710 ABrain7710 left a comment

Choose a reason for hiding this comment

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

There are still a few context managers missing for the engine.connect(). Also the process_ossf_dependency_metrics needs to stay in the secondary collection

augur/api/metrics/pull_request.py Outdated Show resolved Hide resolved
augur/application/db/models/augur_operations.py Outdated Show resolved Hide resolved
augur/tasks/data_analysis/insight_worker/tasks.py Outdated Show resolved Hide resolved
augur/tasks/data_analysis/insight_worker/tasks.py Outdated Show resolved Hide resolved
augur/tasks/data_analysis/insight_worker/tasks.py Outdated Show resolved Hide resolved
augur/tasks/data_analysis/insight_worker/tasks.py Outdated Show resolved Hide resolved
augur/tasks/start_tasks.py Outdated Show resolved Hide resolved
Signed-off-by: Isaac Milarsky <[email protected]>
Copy link
Contributor

@ABrain7710 ABrain7710 left a comment

Choose a reason for hiding this comment

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

Looks good

@sgoggins sgoggins merged commit 6c979f2 into dev Nov 1, 2023
4 checks passed
@IsaacMilarky IsaacMilarky deleted the SQLAlchemy_upgrade branch November 1, 2023 18:07
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.

4 participants