-
Notifications
You must be signed in to change notification settings - Fork 2
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
Fix: IT fails in anvilprod due to multiple values for is_supplementary (#5229) #5231
Fix: IT fails in anvilprod due to multiple values for is_supplementary (#5229) #5231
Conversation
Changes Unknown when pulling 95334f0 on issues/nadove-ucsc/5229-it-fails-anvilprod-multiple-supp into ** on develop**. |
Codecov Report
@@ Coverage Diff @@
## develop #5231 +/- ##
========================================
Coverage 84.38% 84.38%
========================================
Files 149 148 -1
Lines 18312 18271 -41
========================================
- Hits 15452 15418 -34
+ Misses 2860 2853 -7
|
Fix verified by running IT with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming you ran IT on the branch you pushed, I find it difficult to follow how the IT is passing without the fix for #5207 which your branch does not include. Without that fix, the IT frequently fails early due to an oversized partition. Do you know why the IT passes on your branch without that fix?
On my branch (issues/hannes-ucsc/5015-anvilprod), which include this fix and the one for #5207, the IT fails with extra items in the set of indexed bundles, or missing items in the set if expected bundles:
https://gitlab.prod.anvil.gi.ucsc.edu/ucsc/azul/-/jobs/4499
That branch uses pinned seed (6634795309975096822). Could you please re-run IT from your branch, and with that seed?
I will run IT with a random seed on my branch.
Are you using a common prefix in the deployment you ran the IT against? |
Also fails, and in the same way (AssertionError: Items in the first set but not the second). |
I was, but I have now removed it. https://service.nadove5.anvil.gi.ucsc.edu/repository/sources |
I rebased on Subject: [PATCH] fix
---
Index: test/integration_test.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/integration_test.py b/test/integration_test.py
--- a/test/integration_test.py (revision d51d7c94425facef9ca708dc25c6b8fa69faf292)
+++ b/test/integration_test.py (date 1684890437821)
@@ -198,7 +198,7 @@
super().setUp()
# All random operations should be made using this seed so that test
# results are deterministically reproducible
- self.random_seed = randint(0, sys.maxsize)
+ self.random_seed = 6634795309975096822
self.random = Random(self.random_seed)
log.info('Using random seed %r', self.random_seed)
@@ -265,7 +265,7 @@
fqids = self.azul_client.list_bundles(catalog, source, partition_prefix)
bundle_count = len(fqids)
partition = f'Partition {effective_prefix!r} of source {source.spec}'
- if not config.is_sandbox_or_personal_deployment:
+ if True:
# For sources that use partitioning, 512 is the desired partition
# size. In practice, we observe the reindex succeeding with sizes
# >700 without the partition size becoming a limiting factor. From |
dc441c6
to
d51d7c9
Compare
To reproduce @hannes-ucsc 's observed error, we needed to make this change: Subject: [PATCH] fix
---
Index: src/azul/terra.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/azul/terra.py b/src/azul/terra.py
--- a/src/azul/terra.py (revision f6e73608ab5b3f0e4caacfb7ed375cb309256ca8)
+++ b/src/azul/terra.py (date 1685045014024)
@@ -646,7 +646,7 @@
# FIXME: AnVIL deployments conflate indexer and public SAs
# https://github.com/DataBiosphere/azul/issues/4398
service_account=(config.ServiceAccount.indexer
- if config.is_anvil_enabled() and not config.is_hca_enabled()
+ if False
else config.ServiceAccount.public)
)
) to emulate the effect of this commit on his branch. |
d51d7c9
to
9a10cd1
Compare
Files that are incorrectly marked as supplementary were causing the bundles that contain them to be counted twice: once (correctly) as a primary bundle and once (incorrectly) as a supplementary bundle. My first fix didn't work because when looking at the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this change, the IT passes on my branch, too.
Please request a PL slot about this.
9a10cd1
to
77f0b87
Compare
77f0b87
to
95334f0
Compare
Connected issues: #5229
Checklist
Author
develop
issues/<GitHub handle of author>/<issue#>-<slug>
partial
label to PR or this PR completely resolves all connected issues1 when the issue title describes a problem, the corresponding PR
title is
Fix:
followed by the issue titleAuthor (reindex, API changes)
r
tag to commit title or this PR does not require reindexingreindex
label to PR or this PR does not require reindexinga
(compatible changes) orA
(incompatible ones) tag to commit title or this PR does not modify the Azul service APIAPI
label to connected issues or this PR does not modify the Azul service APIAuthor (chains)
base
label to the blocking PR or this PR is not chained to another PRchained
label to this PR or this PR is not chained to another PRAuthor (upgrading)
u
tag to commit title or this PR does not require upgradingupgrade
label to PR or this PR does not require upgradingAuthor (operator tasks)
Author (hotfixes)
F
tag to main commit title or this PR does not include permanent fix for a temporary hotfixprod
branch has no temporary hotfixes for any connected issuesAuthor (before every review)
develop
, squashed old fixupsmake requirements_update
or this PR does not touch requirements*.txt, common.mk, Makefile and DockerfileR
tag to commit title or this PR does not touch requirements*.txtreqs
label to PR or this PR does not touch requirements*.txtmake integration_test
passes in personal deployment or this PR does not touch functionality that could break the ITPeer reviewer (after requesting changes)
Uncheck the Author (before every review) checklists.
Peer reviewer (after approval)
Primary reviewer (after requesting changes)
Uncheck the before every review checklists. Update the
N reviews
label.Primary reviewer (after approval)
demo
orno demo
no demo
no sandbox
N reviews
label is accurateOperator (before pushing merge the commit)
reindex
label andr
commit title tagno demo
upgrade
develop
dev
and addedsandbox
label or PR is labeledno sandbox
anvildev
or PR is labeledno sandbox
sandbox
deployment or PR is labeledno sandbox
anvilbox
deployment or PR is labeledno sandbox
sandbox
deployment or PR is labeledno sandbox
anvilbox
deployment or PR is labeledno sandbox
sandbox
or this PR does not remove catalogs or otherwise causes unreferenced indicesanvilbox
or this PR does not remove catalogs or otherwise causes unreferenced indicessandbox
or this PR does not require reindexingsandbox
anvilbox
or this PR does not require reindexingsandbox
sandbox
or this PR does not require reindexingsandbox
anvilbox
or this PR does not require reindexingsandbox
Operator (after pushing the merge commit)
base
dev
or PR is labeledno sandbox
anvildev
or PR is labeledno sandbox
dev
1dev
1anvildev
1anvildev
1dev
anvildev
1 When pushing the merge commit is skipped due to the PR being
labelled
no sandbox
, the next build triggered by a PR whose merge commit ispushed determines this checklist item.
Operator (reindex)
dev
or this PR does not remove catalogs or otherwise causes unreferenced indicesanvildev
or this PR does not remove catalogs or otherwise causes unreferenced indicesdev
or this PR does not require reindexinganvildev
or this PR does not require reindexingdev
or this PR does not require reindexinganvildev
or this PR does not require reindexingdev
deployment or this PR does not require reindexinganvildev
deployment or this PR does not require reindexingOperator
Shorthand for review comments
L
line is too longW
line wrapping is wrongQ
bad quotesF
other formatting problem