Move code into palace.manager package (PP-1161) #1802

jonathangreen · 2024-04-22T20:31:28Z

Description

Move all the CM files into a namespace package called palace. The cm code is in the palace.manager sub-module.

I started moving some of the code from core into the main palace.manager package, but there is still more work to do there. We will probably want to continue moving things around after this PR, but this one is the most disruptive, since it moves all the code into the package, and moves around our database models.

After this PR all our database models live at: palace.manager.sqlalchemy.model.*. I broke the import cycles that caused the lanes and saml models to previously live elsewhere, so now all our DB models live in that module and importing palace.manager.sqlalchemy.model is enough to initialize them.

The separate customlists package has also been moved into the main palace.manager package.

The large number of removals on this PR are due to fixture files that were removed, since they were not being used anywhere. Since we use relative paths to lookup the fixtures, I had to go through all these files anyway.

I went through and made comments on the PR in the parts of the code that I changed. I attempted to primarily move things around and not change much logic. The only logic changes that were made as part of this PR had to do with loading files based on paths, since the paths changed, and some of them moved into the package.

Motivation and Context

See PP-1161. Its helpful to be able to import our CM code, but previously this was impossible without messing with import paths since we didn't have a reliable package name.

Note: One major downside of this change is we no longer have the nice property of having two packages api and core to allow us to run tests in parallel. So all the tests have to run sequentially now, meaning CI runs take longer. I'm going to make a follow up ticket to see if there is anything we can do to make this a bit faster, since the test runs now take quite a lot longer.

How Has This Been Tested?

Tests in CI
Testing locally
- Note: I'm still doing some local testing to try to make sure everything is working okay

Checklist

I have updated the documentation accordingly.
All new and existing tests passed.

codecov · 2024-04-23T15:24:13Z

Codecov Report

Attention: Patch coverage is 99.81256% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 90.00%. Comparing base (6c52b4c) to head (d30fc4c).

Files	Patch %	Lines
src/palace/manager/api/routes.py	85.71%	1 Missing ⚠️
src/palace/manager/api/util/profilers.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1802      +/-   ##
==========================================
- Coverage   90.31%   90.00%   -0.31%     
==========================================
  Files         259      298      +39     
  Lines       28671    39642   +10971     
  Branches     6521     8597    +2076     
==========================================
+ Hits        25893    35680    +9787     
- Misses       1840     2630     +790     
- Partials      938     1332     +394

Flag	Coverage Δ
Api	`?`
Core	`?`
manager	`89.80% <99.81%> (?)`
migration	`24.60% <40.29%> (+0.66%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

jonathangreen · 2024-04-25T20:13:02Z

I'm still doing some testing locally to try to verify everything is still functional after the re-naming, but I think this is ready for a first round of review when folks have time.

jonathangreen · 2024-04-26T02:04:36Z

.dockerignore

@@ -4,9 +4,6 @@
 # Temp files
 *~

-# Readmes
-**/*.md


Readme needs to be in the docker build for poetry to properly install the package

jonathangreen · 2024-04-26T02:05:20Z

.github/codecov.yml

@@ -11,5 +11,5 @@ comment:

 flag_management:
  default_rules: # the rules that will be followed for any flag added, generally
-    carryforward: true
+    carryforward: false


We changed the flags so if we keep carry forward enabled we will carry the old flags forward forever. I don't think carryforward is giving a lot of value here anyway. It really just masks problems when codecov fails to upload coverage, so I think turning it off makes sense.

jonathangreen · 2024-04-26T02:07:15Z

alembic/env.py

@@ -31,15 +30,6 @@
 # ... etc.


-# Import the models not included in models/__init__.py
-# This is required for autogenerate to work correctly
-from api.saml.metadata.federations.model import (  # noqa: autoflake


This is no longer necessary because all our models are in the same package and the init.py imports them

jonathangreen · 2024-04-26T12:01:31Z

api/integration/registry/metadata.py

-        super().__init__(Goals.METADATA_GOAL)
-
-        self.register(NYTBestSellerAPI, canonical="New York Times")
-        self.register(NoveListAPI, canonical="NoveList Select")


The import changes were enough in this file that it's marked as removed, but the file still exists. It's just being counted by git as a removal and an add instead of a move.

jonathangreen · 2024-04-26T12:05:30Z

bin/run

@@ -88,51 +91,40 @@ create_pidfile () {
 create_dir $piddir

 # Check that the script exists.
-FULL_SCRIPT_PATH=$LIBSIMPLE_DIR/bin/$SCRIPT_PATH
+FULL_SCRIPT_PATH=$PALACE_DIR/bin/$SCRIPT_PATH
 if [[ ! -f $FULL_SCRIPT_PATH ]]; then


Core/bin no longer exists so this script needed to be updated

jonathangreen · 2024-04-26T13:02:59Z

src/palace/manager/util/resources.py

+from importlib.resources import files
+
+
+def resources_dir(subdir: str) -> Traversable:


We use this new function to load any content needed from the package. It uses importlib to get the path to the package. This handles the various cases where our code could end up in different package locations.

jonathangreen · 2024-04-26T13:06:49Z

tests/fixtures/files.py


-    """These are files that are served by the CM in production, and aren't test suite specific."""
+class FilesFixture:


This fixture is updated for the new import paths, and a number of the other separate files fixtures have all been centralized in this file.

jonathangreen · 2024-04-26T13:09:09Z

tests/manager/api/test_app.py

+        initialize_application()
+
+    # Make sure that the HTTP configuration was set
+    mock_set_quick_failure_settings.assert_called_once()


This test was updated to use a mock. Previously the call it was making to initialize_application was changing the global state for HTTP module, which caused other tests to fail. Using a mock here does the same test, but prevents this side effect.

jonathangreen · 2024-04-26T13:13:17Z

tests/mocks/flask.py

+from flask_pydantic_spec.utils import parse_multi_dict
+
+
+def add_request_context(request, model, form=None) -> None:


This isn't new, it was just moved out of the tests package into mocks.

jonathangreen · 2024-04-26T13:14:29Z

tests/mocks/sip.py

+from palace.manager.api.sip.client import Constants
+
+
+class MockSIPClient(SIPClient):


This isn't new, but it was in the main package, so it got moved out to mocks as part of this re-org.

jonathangreen · 2024-04-26T13:21:04Z

src/palace/manager/customlists/explain.py

@@ -31,12 +31,6 @@ def _parse_arguments(args: list[str]) -> argparse.Namespace:
            default=0,
            help="Increase verbosity",
        )
-        parser.add_argument(
-            "--report-schema-file",


Since these schema files are part of the package, it doesn't really make sense to pass them into the script anymore. Instead we load them from the package directly.

jonathangreen · 2024-04-26T13:24:44Z

Codecov is reporting that coverage has decreased due to this PR. It looks like that is because we are now reporting coverage on all the files in the palace.manager package, which adds an additional 39 files that coverage is monitoring. I these additional files account for the decrease in coverage.

jonathangreen · 2024-04-26T15:38:12Z

Pushed one small commit just now d30fc4c that resolves some issues I found when testing in the docker containers locally. These issues were causing the scripts not to run in the scripts container.

dbernstein

dbernstein · 2024-04-26T17:29:22Z

bin/run

@@ -1,12 +1,12 @@
 #!/bin/bash
 #
-# Ensures that Library Simplified Circulation Manager scripts don't run
+# Ensures that Palace Manager scripts don't run


Is there any reason for us not to abandon this script and instead using something that already does this like flock?

Probably not worth changing it at this point, since it's used to run scripts via cron and those are going to be moved to the queue-based approach.

I think it makes sense to keep whats here like @tdilauro mentioned, since this is going away soon. Nice to know flock exists though, I hadn't heard of it before, so I appreciate the tip.

tdilauro · 2024-04-26T18:02:07Z

core/resources/lcc_one_level.json~

This looks like a temporary editing file. Should it be here?

No it shouldn't. That is why the PR is deleting it 😄

Looks like its been in the repo for a looong time.

tdilauro · 2024-04-26T18:03:23Z

core/resources/random_isbns.txt~

This looks like a temporary editing file. Should it be here?

No it shouldn't. That is why the PR is deleting it 😄

Looks like its been in the repo for a looong time.

tdilauro · 2024-04-26T18:07:55Z

bin/run

@@ -1,12 +1,12 @@
 #!/bin/bash
 #
-# Ensures that Library Simplified Circulation Manager scripts don't run
+# Ensures that Palace Manager scripts don't run


Probably not worth changing it at this point, since it's used to run scripts via cron and those are going to be moved to the queue-based approach.

jonathangreen force-pushed the feature/put-into-package branch from 3e1c27a to 9a953d0 Compare April 25, 2024 19:06

jonathangreen requested a review from a team April 25, 2024 20:13

jonathangreen changed the title ~~Move code into palace.manager package~~ Move code into palace.manager package (PP-1161) Apr 25, 2024

jonathangreen force-pushed the feature/put-into-package branch from d43df19 to 760c4e7 Compare April 25, 2024 22:11

jonathangreen commented Apr 26, 2024

View reviewed changes

jonathangreen mentioned this pull request Apr 26, 2024

Fix unreleated unresolved symbols (PP-1161) #1808

Merged

2 tasks

jonathangreen added 6 commits April 26, 2024 12:36

Move around all the CM files to put them into package

fdc1744

Small update to tox config

188cae8

Remove __init__.py

4628a25

Update readme

22c14d4

Disable carryforward flag

23ddd7d

Fix issues with scripts container

d30fc4c

jonathangreen force-pushed the feature/put-into-package branch from b1d07ad to d30fc4c Compare April 26, 2024 15:37

jonathangreen marked this pull request as ready for review April 26, 2024 16:08

dbernstein approved these changes Apr 26, 2024

View reviewed changes

tdilauro reviewed Apr 26, 2024

View reviewed changes

jonathangreen merged commit b1ab61a into main Apr 26, 2024
20 of 21 checks passed

jonathangreen deleted the feature/put-into-package branch April 26, 2024 19:02

jonathangreen mentioned this pull request Apr 29, 2024

Override Celery automatic task naming #1809

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move code into palace.manager package (PP-1161) #1802

Move code into palace.manager package (PP-1161) #1802

jonathangreen commented Apr 22, 2024 •

edited

Loading

codecov bot commented Apr 23, 2024 •

edited

Loading

jonathangreen commented Apr 25, 2024

jonathangreen Apr 26, 2024

jonathangreen Apr 26, 2024 •

edited

Loading

jonathangreen Apr 26, 2024

jonathangreen Apr 26, 2024 •

edited

Loading

jonathangreen Apr 26, 2024

jonathangreen Apr 26, 2024

jonathangreen Apr 26, 2024

jonathangreen Apr 26, 2024

jonathangreen Apr 26, 2024

jonathangreen Apr 26, 2024

jonathangreen Apr 26, 2024

jonathangreen commented Apr 26, 2024

jonathangreen commented Apr 26, 2024

dbernstein left a comment

dbernstein Apr 26, 2024

tdilauro Apr 26, 2024

jonathangreen Apr 26, 2024

tdilauro Apr 26, 2024

jonathangreen Apr 26, 2024

tdilauro Apr 26, 2024

jonathangreen Apr 26, 2024

tdilauro Apr 26, 2024

		from importlib.resources import files


		def resources_dir(subdir: str) -> Traversable:


		"""These are files that are served by the CM in production, and aren't test suite specific."""
		class FilesFixture:

		from flask_pydantic_spec.utils import parse_multi_dict


		def add_request_context(request, model, form=None) -> None:

		from palace.manager.api.sip.client import Constants


		class MockSIPClient(SIPClient):

Move code into palace.manager package (PP-1161) #1802

Move code into palace.manager package (PP-1161) #1802

Conversation

jonathangreen commented Apr 22, 2024 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Checklist

codecov bot commented Apr 23, 2024 • edited Loading

Codecov Report

jonathangreen commented Apr 25, 2024

Choose a reason for hiding this comment

jonathangreen Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathangreen Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathangreen commented Apr 26, 2024

jonathangreen commented Apr 26, 2024

dbernstein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathangreen commented Apr 22, 2024 •

edited

Loading

codecov bot commented Apr 23, 2024 •

edited

Loading

jonathangreen Apr 26, 2024 •

edited

Loading

jonathangreen Apr 26, 2024 •

edited

Loading