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

Remove ert-storage as dependency #6559

Merged
merged 1 commit into from
Nov 29, 2023
Merged

Remove ert-storage as dependency #6559

merged 1 commit into from
Nov 29, 2023

Conversation

dafeda
Copy link
Contributor

@dafeda dafeda commented Nov 13, 2023 •
edited
Loading

Pre review checklist

  • Read through the code changes carefully after finishing work
  • Make sure tests pass locally (after every commit!)
  • Prepare changes in small commits for more convenient review (optional)
  • PR title captures the intent of the changes, and is fitting for release notes.
  • Updated documentation
  • Ensured that unit tests are added for all new behavior (See
    Ground Rules),
    and changes to existing code have good test coverage.

Pre merge checklist

  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.

@dafeda dafeda self-assigned this Nov 13, 2023
@dafeda dafeda added the release-notes:skip If there should be no mention of this in release notes label Nov 13, 2023
@dafeda dafeda force-pushed the ert_storage branch 2 times, most recently from 4da99e0 to 9c6a94d Compare November 13, 2023 18:33
@dafeda dafeda changed the title Commitin Remove ert-storage as dependency Nov 13, 2023
@dafeda dafeda force-pushed the ert_storage branch 4 times, most recently from 5b78713 to d7b56ca Compare November 15, 2023 12:23
@dafeda dafeda marked this pull request as ready for review November 22, 2023 08:49
@frode-aarstad
Copy link
Contributor

Should we also use the opportunity to rename dark_storage ?

frode-aarstad
frode-aarstad reviewed Nov 22, 2023
View reviewed changes
src/ert/services/_storage_main.py Outdated
# Dark Storage imports from ERT Storage, which connects to the database
# at startup. We set the database URL to an SQLite in-memory database so
# that the import succeeds.
os.environ["ERT_STORAGE_DATABASE_URL"] = "sqlite://"
Copy link
Contributor

Choose a reason for hiding this comment

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

Think we can remove the DB references. There are also some references to sqlite around the code

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. Dark storage doesn't use a traditional RDBMS.

This particular line is a hack to get ert-storage to initialise, as the database connection is made during import. Particularly, we needed to determine whether the we are using SQLite or Postgresql, and modify the schema accordingly. Because Dark Storage imports from ert-storage, it'd import the database schema which requires the ERT_STORAGE_DATABASE_URL to be set. We set it to sqlite:// here, which specifies to create an in-memory sqlite database.

@dafeda dafeda requested a review from pinkwah November 27, 2023 07:14
pinkwah
pinkwah reviewed Nov 27, 2023
View reviewed changes
Copy link
Contributor

@pinkwah pinkwah left a comment

Choose a reason for hiding this comment

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

Please also put a link to the github repo and the commit from which you took this code, so that the git history is at least somewhat preserved.

src/ert/services/_storage_main.py Outdated
# Dark Storage imports from ERT Storage, which connects to the database
# at startup. We set the database URL to an SQLite in-memory database so
# that the import succeeds.
os.environ["ERT_STORAGE_DATABASE_URL"] = "sqlite://"
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. Dark storage doesn't use a traditional RDBMS.

This particular line is a hack to get ert-storage to initialise, as the database connection is made during import. Particularly, we needed to determine whether the we are using SQLite or Postgresql, and modify the schema accordingly. Because Dark Storage imports from ert-storage, it'd import the database schema which requires the ERT_STORAGE_DATABASE_URL to be set. We set it to sqlite:// here, which specifies to create an in-memory sqlite database.

@pinkwah
Copy link
Contributor

pinkwah commented Nov 27, 2023

Should we also use the opportunity to rename dark_storage ?

I would like to at least add _server as postfix. I personally think _storage_server would be most descriptive of what this is supposed to be. Or _onprem_storage_server, to distinguish this from a possible future separate service that we would like to switch to.

@dafeda
Copy link
Contributor Author

dafeda commented Nov 28, 2023

Should we also use the opportunity to rename dark_storage ?

I'd rather we do that in a separate PR I think.

@codecov-commenter
Copy link

codecov-commenter commented Nov 28, 2023 •
edited
Loading

Codecov Report

Attention: 47 lines in your changes are missing coverage. Please review.

Comparison is base (35d1984) 83.57% compared to head (2c0be51) 83.57%.
Report is 2 commits behind head on main.

Files Patch % Lines
src/ert/dark_storage/client/_session.py 33.33% 22 Missing ⚠️
src/ert/dark_storage/client/async_client.py 45.45% 6 Missing ⚠️
src/ert/dark_storage/security.py 60.00% 6 Missing ⚠️
src/ert/dark_storage/app.py 73.33% 4 Missing ⚠️
src/ert/dark_storage/json_schema/ensemble.py 87.50% 3 Missing ⚠️
src/ert/services/_storage_main.py 0.00% 2 Missing ⚠️
src/ert/dark_storage/client/client.py 90.90% 1 Missing ⚠️
src/ert/dark_storage/compute/misfits.py 93.75% 1 Missing ⚠️
src/ert/dark_storage/exceptions.py 92.85% 1 Missing ⚠️
src/ert/dark_storage/json_schema/prior.py 98.24% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff            @@
##             main    #6559    +/-   ##
========================================
  Coverage   83.57%   83.57%            
========================================
  Files         345      359    +14     
  Lines       20761    21018   +257     
  Branches      948      948            
========================================
+ Hits        17350    17565   +215     
- Misses       3117     3159    +42     
  Partials      294      294

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dafeda
Copy link
Contributor Author

dafeda commented Nov 29, 2023

Please also put a link to the github repo and the commit from which you took this code, so that the git history is at least somewhat preserved.

I've squashed and added a link to the latest commit from ert-storage in the commit message body.

@dafeda dafeda merged commit b0ef3f5 into equinor:main Nov 29, 2023
43 checks passed
@dafeda dafeda deleted the ert_storage branch November 29, 2023 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frode-aarstad frode-aarstad frode-aarstad left review comments

@pinkwah pinkwah pinkwah approved these changes

Assignees

@dafeda dafeda

Labels
release-notes:skip If there should be no mention of this in release notes
Projects
SCOUT
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dafeda @frode-aarstad @pinkwah @codecov-commenter