Improve CO save performance for bulk updates#8057
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces thread-local caching for business-rule and permission queries to address database query performance bottlenecks. It adds a ChangesCaching infrastructure and business-rule optimization
Redis module consolidation and documentation
Frontend helpers and business-rule tests
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
specifyweb/backend/businessrules/utils.py (1)
33-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid caching the fallback
Falseafter a preference read/parsing failure.Right now any exception path leaves
unique_catnum_enabled = Falseand still stores that value in the context cache. A single transientget_app_resource(...)/JSON failure would then disable the uniqueness rule for every later save in the same bulk operation.Suggested fix
def get_unique_catnum_across_comp_co_coll_pref(collection, user) -> bool: import specifyweb.backend.context.app_resource as app_resource cache = _unique_catnum_pref_cache.get() @@ - unique_catnum_enabled: bool = False + unique_catnum_enabled: bool = False + cacheable = True @@ except json.JSONDecodeError: + cacheable = False logger.warning(f"Error: Could not decode JSON for collection preferences") except TypeError as e: + cacheable = False logger.warning(f"Error: Unexpected data structure in collection preferences: {e}") except Exception as e: + cacheable = False logger.warning(f"An unexpected error occurred: {e}") - if cache is not None: + if cache is not None and cacheable: cache[cache_key] = unique_catnum_enabled🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/backend/businessrules/utils.py` around lines 33 - 57, The current try/except always leaves unique_catnum_enabled=False on errors and then writes that fallback into cache; change this so the cache is only updated when the preference read/parse actually succeeded. For example, after calling app_resource.get_app_resource and json.loads and validating the nested keys (the logic that computes unique_catnum_enabled from collection_prefs_json, unique_catalog_number_pref and behavior), set a local success flag (or move the cache write) and only assign cache[cache_key] = unique_catnum_enabled when success is True and cache is not None; do not write to cache inside any except path that handles JSONDecodeError/TypeError/Exception. Ensure references: unique_catnum_enabled, cache, cache_key, app_resource.get_app_resource, collection_prefs_json, json.loads, unique_catalog_number_pref, behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@specifyweb/backend/businessrules/utils.py`:
- Around line 33-57: The current try/except always leaves
unique_catnum_enabled=False on errors and then writes that fallback into cache;
change this so the cache is only updated when the preference read/parse actually
succeeded. For example, after calling app_resource.get_app_resource and
json.loads and validating the nested keys (the logic that computes
unique_catnum_enabled from collection_prefs_json, unique_catalog_number_pref and
behavior), set a local success flag (or move the cache write) and only assign
cache[cache_key] = unique_catnum_enabled when success is True and cache is not
None; do not write to cache inside any except path that handles
JSONDecodeError/TypeError/Exception. Ensure references: unique_catnum_enabled,
cache, cache_key, app_resource.get_app_resource, collection_prefs_json,
json.loads, unique_catalog_number_pref, behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b811e07-b495-4def-9f5b-ac142c11a48f
📒 Files selected for processing (5)
specifyweb/backend/bulk_copy/bulk_copy.pyspecifyweb/backend/businessrules/rules/collectionobject_rules.pyspecifyweb/backend/businessrules/utils.pyspecifyweb/backend/workbench/upload/upload.pyspecifyweb/specify/api/dispatch.py
|
Merci, Monsieur Lapin 🐰 |
|
If this fix isn't enough of a speed up, I have another solution idea that involves a schema migration for adding an index on the Component.catalognumber field: operations = [
migrations.AddIndex(
model_name="component",
index=models.Index(
fields=["catalognumber"],
name="ComponentCatNumIDX",
),
),
] |
bhumikaguptaa
left a comment
bhumikaguptaa
left a comment
There was a problem hiding this comment.
- In Batch Edit, update a non-catalognumber field across many Collection Objects and confirm the operation completes noticeably faster than before.
- In Data Set commit, create or update Collection Objects and confirm progress advances steadily without the previous long delay.
- Create a Collection Object with a catalognumber that matches a Component in the same collection and confirm the duplicate warning still appears.
- Create or update a Collection Object with a catalog number that matches a Component in a different collection and confirm it is allowed.
- Confirm regular Collection Object create/edit workflows still save successfully.
Everything works as expected except when I tried to upload a dataset with approximately 12000 records via Workbench, it took around 20 minutes (and is still going on) to validate and then upload the dataset. The time it took is similar to the time Demo fish/main took. I am unsure if it is supposed to take this much time.
kwhuber
left a comment
kwhuber
left a comment
There was a problem hiding this comment.
-
In Batch Edit, update a non-catalognumber field across many Collection Objects and confirm the operation completes noticeably faster than before.
-
In Data Set commit, create or update Collection Objects and confirm progress advances steadily without the previous long delay.
-
Create a Collection Object with a catalognumber that matches a Component in the same collection and confirm the duplicate warning still appears.
-
Create or update a Collection Object with a catalog number that matches a Component in a different collection and confirm it is allowed.
-
Confirm regular Collection Object create/edit workflows still save successfully.
-
For Data set commit, I noticed marginal improvement, but it wasn't too high of a wait to begin with. I altered a comments field of ~4800 CO records. I tested Data set manipulation on uw_geo and on main experienced 17 records committed every 3 seconds; meanwhile on the issue-8055 branch I noticed about 20-25 records committed every 3 seconds so slightly quicker.
|
I worked some more on trying to speed up the data set commit by adding some caching functionality. I updated the fix so the bulk/dataset context now caches Component catalog numbers per collection. Good to test out the speed of the data set commit again. If the caching solution looks good, then maybe we can discus if adding a index to the component.catalognumber field would be better. |
|
Updated 1153 COs in less than 5 min |
Iwantexpresso
left a comment
Iwantexpresso
left a comment
There was a problem hiding this comment.
-
In Batch Edit, update a non-catalognumber field across many Collection Objects and confirm the operation completes noticeably faster than before.
-
In Data Set commit, create or update Collection Objects and confirm progress advances steadily without the previous long delay.
-
Create a Collection Object with a catalognumber that matches a Component in the same collection and confirm the duplicate warning still appears.
-
Create or update a Collection Object with a catalog number that matches a Component in a different collection and confirm it is allowed.
-
Confirm regular Collection Object create/edit workflows still save successfully.
dataset validation and Upload of 4810 records completed in under 2 minutes
Uniqueness: Preference to have catalog number unique across CO and Component table
-
Verify there is a save blocker saying that the cat num value is already in use
-
Verify there is also a save blocker when using another component cat num value
looking good so far!
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specifyweb/backend/businessrules/utils.py`:
- Around line 194-196: The broad except Exception that logs and returns
unique_catnum_enabled should be tightened: replace the catch-all with only the
expected parsing/shape exceptions (e.g., ValueError, TypeError,
json.JSONDecodeError or the specific validation exception your parser raises) in
the try/except around the uniqueness-preference resolution, and either re-raise
any other exceptions or remove the generic except so unexpected errors propagate
(or raise a clear FatalError) instead of silently defaulting uniqueness to
False; update the logging in the handled-exception branch to include context
(function name and the offending input) and ensure the variable
unique_catnum_enabled is only returned after successful, validated resolution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f2d1fc72-6b15-43c3-8fb8-e1bc832d1fa4
📒 Files selected for processing (8)
specifyweb/backend/businessrules/rules/collectionobject_rules.pyspecifyweb/backend/businessrules/tests/test_collectionobject.pyspecifyweb/backend/businessrules/tests/test_uniqueness_rules.pyspecifyweb/backend/businessrules/uniqueness_rules.pyspecifyweb/backend/businessrules/utils.pyspecifyweb/backend/permissions/tests.pyspecifyweb/backend/workbench/upload/tests/test_batch_edit_table.pyspecifyweb/backend/workbench/upload/upload.py
🚧 Files skipped from review as they are similar to previous changes (2)
- specifyweb/backend/businessrules/rules/collectionobject_rules.py
- specifyweb/backend/workbench/upload/upload.py
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specifyweb/backend/businessrules/uniqueness_rules.py`:
- Around line 277-280: The fast-path guard in the uniqueness check (the if using
instance.pk and changed_fields_include(instance, rule.all_fields)) ignores
fields that determine whether a rule applies (see _rule_applies_to_instance),
such as discipline/collection, allowing re-parenting to bypass validation;
update the guard to also include those applicability-driving fields (e.g., the
discipline/collection fields or a rule-provided list like
rule.applicability_fields) in the changed_fields_include test, or alternatively
disable the fast path for non-global rules (e.g., when not rule.is_global) so
that _rule_applies_to_instance is always re-evaluated on updates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7bd49f5f-82eb-4ef5-a6be-cb5c712bfadc
📒 Files selected for processing (8)
specifyweb/backend/businessrules/rules/collectionobject_rules.pyspecifyweb/backend/businessrules/tests/test_collectionobject.pyspecifyweb/backend/businessrules/tests/test_uniqueness_rules.pyspecifyweb/backend/businessrules/uniqueness_rules.pyspecifyweb/backend/businessrules/utils.pyspecifyweb/backend/permissions/tests.pyspecifyweb/backend/workbench/upload/tests/test_batch_edit_table.pyspecifyweb/backend/workbench/upload/upload.py
🚧 Files skipped from review as they are similar to previous changes (6)
- specifyweb/backend/permissions/tests.py
- specifyweb/backend/businessrules/rules/collectionobject_rules.py
- specifyweb/backend/businessrules/tests/test_collectionobject.py
- specifyweb/backend/workbench/upload/upload.py
- specifyweb/backend/workbench/upload/tests/test_batch_edit_table.py
- specifyweb/backend/businessrules/utils.py
|
Hi all, to keep this PR somewhat short I've moved the BatchEdit specific changes to issue-8055-bulk-batch-edit . Unfortunately, I think there might still be some more work regarding UniquenessRules to be done with this PR. You can observe that since The performance optimizations thus far are targeted to the fetching of the rule: not the calculation of the rule itself. Specifically, I'm pretty sure the slowdown when checking Uniqueness Rules from specify7/specifyweb/backend/businessrules/uniqueness_rules.py Lines 195 to 205 in c8acd36 I have some ideas and a general design in mind when it comes to optimizing the checking of uniqueness rules for an arbitrary amount of rules. Currently, the number of uniqueness rules someone has defined can significantly increase the time it takes to evaluate the rules: we can significantly reduce this time by batching the separate queries for each rule into a single query for all rules (i.e., eliminate the "psuedo" N + 1 problem). I do not yet have a solution or design when it comes to the case-sensitivity problem introduced in #7470 (maybe we can do the comparison outside of the database. That is, fetch a superset of values we would already be fetching from the database and reduce the set in the backend). |
@melton-jason says "this is 90+% of the bottleneck" We need to remove this. It is not proven, no use cases have been given, and it would simply revert to the behavior it had prior to 7.12. Jason will remove and test. We can come up with a more narrow fix for #3920 (comment), but it was reported only once in years. Can you open a PR for |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specifyweb/backend/businessrules/uniqueness_rules.py`:
- Around line 104-118: The function _cached_businessrules_migration_applied
incorrectly treats the cache "is_set" sentinel as truthy and can return True for
an unmigrated state; update the conditional in
_cached_businessrules_migration_applied to only return the cached value when the
cache is active and the retrieved marker is not the missing sentinel (use
_uniqueness_migration_cache.MISSING to check), i.e., check "cache_is_active and
is_set is not _uniqueness_migration_cache.MISSING" before returning; keep the
rest of the logic (calling _initial_businessrules_migration_applied and setting
the cache when is_applied is True) unchanged.
In `@specifyweb/backend/context/remote_prefs.py`:
- Around line 59-63: In get_preference, the regex treats dots as metacharacters
and uses a greedy (.+) that disallows empty values and can overrun into
following preferences; change the pattern to escape the key (use re.escape on
the key), anchor it so it matches either start or a delimiter (e.g. start or
';') before the key, and capture up to the next delimiter or end allowing empty
values (use a non-greedy or character-class like [^;]*); update the re.search in
get_preference to use that new pattern and keep returning match.group(1) when
present.
In `@specifyweb/backend/workbench/upload/auditlog.py`:
- Around line 153-161: Validate and sanitize audit_lifespan from
get_global_pref('AUDIT_LIFESPAN_MONTHS') by parsing it to an integer and
ensuring it is non-negative, then compute a cutoff datetime in Python (e.g.,
current date minus that many months using datetime/relativedelta) instead of
concatenating into SQL; use a parameterized cursor.execute for both delete
statements against spauditlogfield and spauditlog with a WHERE timestampcreated
< %s and pass the computed cutoff value as the parameter to prevent SQL
injection and avoid calling .lower() on the raw preference.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d3b8aa45-e4a8-4b40-8710-5a9284f70621
📒 Files selected for processing (13)
specifyweb/backend/bulk_copy/bulk_copy.pyspecifyweb/backend/businessrules/rules/collectionobject_rules.pyspecifyweb/backend/businessrules/tests/test_uniqueness_rules.pyspecifyweb/backend/businessrules/uniqueness_rules.pyspecifyweb/backend/businessrules/utils.pyspecifyweb/backend/context/remote_prefs.pyspecifyweb/backend/context/views.pyspecifyweb/backend/permissions/permissions.pyspecifyweb/backend/stored_queries/format.pyspecifyweb/backend/workbench/upload/auditlog.pyspecifyweb/backend/workbench/upload/upload.pyspecifyweb/frontend/js_src/lib/components/BatchEdit/index.tsxspecifyweb/specify/utils/scoping.py
🚧 Files skipped from review as they are similar to previous changes (3)
- specifyweb/backend/bulk_copy/bulk_copy.py
- specifyweb/backend/businessrules/tests/test_uniqueness_rules.py
- specifyweb/backend/businessrules/rules/collectionobject_rules.py
8 participants
Fixes #8055
The new CollectionObject/Component catalognumber uniqueness rule was running during every CollectionObject save, even when the catalognumber and collection were unchanged. These catalognumber checks seemed to be unnecessary likely the main cause of the slowdown during batch edits, data set commits, and bulk copy operations.
The most straight forward fix seemed to be skipping the component catalognumber duplicate check when a CO save does not change
catalognumberorcollection. I also added caching to theCollectionPreferencesuniqueness setting during bulk create, bulk copy, and data set upload operations. One other thing that might add some speed up is scoping component duplicate lookups to just the current collection? I added that for now, but might remove it if it doesn't provide much speedup or needs to remain unique between collections.Still need to do some more testing and performance checking to confirm this solution completely solves the issue.
Checklist
self-explanatory (or properly documented)
Testing instructions
#8055 (comment)
Test cat num uniqueness feat from #6708
Uniqueness: Preference to have catalog number unique across CO and Component table
"uniqueCatalogNumberAccrossComponentAndCO": {"behavior": {"uniqueness": true}}
Summary by CodeRabbit
Performance Improvements
Improvements
Chores