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

[MRG] migrate command (update old indexes) #494

Merged
merged 9 commits into from Jun 22, 2018
Merged

[MRG] migrate command (update old indexes) #494

merged 9 commits into from Jun 22, 2018

Conversation

luizirber
Copy link
Member

@luizirber luizirber commented Jun 12, 2018

This PR adds the migrate command to update old indexes to newer versions.

  • If using an old database there will be a "WARNING" pointing how slow things are going to be. (Should we just stop running and throw an error instead?)
  • Rewrote the metadata updates using a fill_up method (a BFS starting from the leaves).
  • add another logging level, debug (and we should start using the logging module from stdlib...)
  • add an extra argument to save: structure_only. If set, avoid re-saving the data and only updates the SBT JSON.
  • Fix a bug with tree repair and min_n_below interactions.

Checklist

  • Is it mergeable?
  • make test Did it pass the tests?
  • make coverage Is the new code covered?
  • Did it change the command-line interface? Only additions are allowed
    without a major version increment. Changing file formats also requires a
    major version number increment.
  • Was a spellchecker run on the source code and documentation after
    changes were made?

@codecov-io
Copy link

codecov-io commented Jun 12, 2018

Codecov Report

Merging #494 into master will increase coverage by 0.17%.
The diff coverage is 93.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #494      +/-   ##
==========================================
+ Coverage   90.51%   90.69%   +0.17%     
==========================================
  Files          33       33              
  Lines        4936     4999      +63     
  Branches       36       36              
==========================================
+ Hits         4468     4534      +66     
+ Misses        467      464       -3     
  Partials        1        1
Impacted Files Coverage Δ
sourmash/__main__.py 95.83% <ø> (ø) ⬆️
sourmash/sbtmh.py 86.56% <100%> (+0.2%) ⬆️
sourmash/commands.py 91.58% <100%> (+0.08%) ⬆️
sourmash/logging.py 98% <71.42%> (-2%) ⬇️
sourmash/sbt.py 85.88% <94.28%> (+2.55%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a49456c...b39209f. Read the comment docs.

@luizirber luizirber changed the title [WIP] migrate command (update old indexes) [MRG] migrate command (update old indexes) Jun 15, 2018
@luizirber luizirber requested a review from ctb June 15, 2018 20:11
@luizirber
Copy link
Member Author

Ready for review and merge @ctb

Copy link
Contributor

@ctb ctb left a comment

Choose a reason for hiding this comment

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

A few comments -

  • there is no explicit test of the sourmash migrate command. I think we should have one!
  • the warning message about using sourmash migrate is printed even when running sourmash migrate which seems odd
  • the --output flag is odd. I would have thought that this would be the new name of the SBT, not the (I'm guessing here) size of the bloom filters in the new SBT.

We should also add explicit testing of loading old SBTs in here somewhere, but that could be a new issue that we handle separately.

@luizirber
Copy link
Member Author

  • Test added. It will probably need to be updated for each new version of the DB, but I guess this is fine (since new versions of DB might change internal details)
  • added print_version_warning along the load() calls, defaults to True but we disable it for sourmash migrate
  • of course it's odd, it's copy-pasta from another command =P. Removed.

On the explicit loading of older SBTs: despite having the _load_v{1,2} code, we have bigger problems (like v1 assuming the leaves were YAML signatures, which we removed support for loading when we adopted JSON). Not sure what is the best way: supporting it all adds a fairly large maintenance burden, but do we really have users with a v1 database around?

@luizirber luizirber requested a review from ctb June 21, 2018 20:57
@luizirber luizirber merged commit 016e17e into master Jun 22, 2018
@luizirber luizirber deleted the cmd/migrate branch June 23, 2018 01:50
ctb pushed a commit that referenced this pull request Oct 28, 2022
Updates the requirements on
[pytest-cov](https://github.com/pytest-dev/pytest-cov) to permit the
latest version.
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a
href="https://github.com/pytest-dev/pytest-cov/blob/master/CHANGELOG.rst">pytest-cov's
changelog</a>.</em></p>
<blockquote>
<h2>4.0.0 (2022-09-28)</h2>
<p><strong>Note that this release drops support for
multiprocessing.</strong></p>
<ul>
<li>
<p><code>--cov-fail-under</code> no longer causes <code>pytest
--collect-only</code> to fail
Contributed by Zac Hatfield-Dodds in
<code>[#511](pytest-dev/pytest-cov#511)
&lt;https://github.com/pytest-dev/pytest-cov/pull/511&gt;</code>_.</p>
</li>
<li>
<p>Dropped support for multiprocessing (mostly because <code>issue 82408
&lt;https://github.com/python/cpython/issues/82408&gt;</code>_). This
feature was
mostly working but very broken in certain scenarios and made the test
suite very flaky and slow.</p>
<p>There is builtin multiprocessing support in coverage and you can
migrate to that. All you need is this in your
<code>.coveragerc</code>::</p>
<p>[run]
concurrency = multiprocessing
parallel = true
sigterm = true</p>
</li>
<li>
<p>Fixed deprecation in <code>setup.py</code> by trying to import
setuptools before distutils.
Contributed by Ben Greiner in
<code>[#545](pytest-dev/pytest-cov#545)
&lt;https://github.com/pytest-dev/pytest-cov/pull/545&gt;</code>_.</p>
</li>
<li>
<p>Removed undesirable new lines that were displayed while reporting was
disabled.
Contributed by Delgan in
<code>[#540](pytest-dev/pytest-cov#540)
&lt;https://github.com/pytest-dev/pytest-cov/pull/540&gt;</code>_.</p>
</li>
<li>
<p>Documentation fixes.
Contributed by Andre Brisco in
<code>[#543](pytest-dev/pytest-cov#543)
&lt;https://github.com/pytest-dev/pytest-cov/pull/543&gt;</code>_
and Colin O'Dell in
<code>[#525](pytest-dev/pytest-cov#525)
&lt;https://github.com/pytest-dev/pytest-cov/pull/525&gt;</code>_.</p>
</li>
<li>
<p>Added support for LCOV output format via
<code>--cov-report=lcov</code>. Only works with coverage 6.3+.
Contributed by Christian Fetzer in
<code>[#536](pytest-dev/pytest-cov#536)
&lt;https://github.com/pytest-dev/pytest-cov/issues/536&gt;</code>_.</p>
</li>
<li>
<p>Modernized pytest hook implementation.
Contributed by Bruno Oliveira in
<code>[#549](pytest-dev/pytest-cov#549)
&lt;https://github.com/pytest-dev/pytest-cov/pull/549&gt;</code>_
and Ronny Pfannschmidt in
<code>[#550](pytest-dev/pytest-cov#550)
&lt;https://github.com/pytest-dev/pytest-cov/pull/550&gt;</code>_.</p>
</li>
</ul>
<h2>3.0.0 (2021-10-04)</h2>
<p><strong>Note that this release drops support for Python 2.7 and
Python 3.5.</strong></p>
<ul>
<li>Added support for Python 3.10 and updated various test dependencies.
Contributed by Hugo van Kemenade in
<code>[#500](pytest-dev/pytest-cov#500)
&lt;https://github.com/pytest-dev/pytest-cov/pull/500&gt;</code>_.</li>
<li>Switched from Travis CI to GitHub Actions. Contributed by Hugo van
Kemenade in
<code>[#494](pytest-dev/pytest-cov#494)
&lt;https://github.com/pytest-dev/pytest-cov/pull/494&gt;</code>_ and
<code>[#495](pytest-dev/pytest-cov#495)
&lt;https://github.com/pytest-dev/pytest-cov/pull/495&gt;</code>_.</li>
<li>Add a <code>--cov-reset</code> CLI option.
Contributed by Danilo Šegan in
<code>[#459](pytest-dev/pytest-cov#459)
&lt;https://github.com/pytest-dev/pytest-cov/pull/459&gt;</code>_.</li>
<li>Improved validation of <code>--cov-fail-under</code> CLI option.
Contributed by ... Ronny Pfannschmidt's desire for skark in
<code>[#480](pytest-dev/pytest-cov#480)
&lt;https://github.com/pytest-dev/pytest-cov/pull/480&gt;</code>_.</li>
<li>Dropped Python 2.7 support.</li>
</ul>
<!-- raw HTML omitted -->
</blockquote>
<p>... (truncated)</p>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/28db055bebbf3ee016a2144c8b69dd7b80b48cc5"><code>28db055</code></a>
Bump version: 3.0.0 → 4.0.0</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/57e9354a86f658556fe6f15f07625c4b9a9ddf53"><code>57e9354</code></a>
Really update the changelog.</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/56b810b91c9ae15d1462633c6a8a1b522ebf8e65"><code>56b810b</code></a>
Update chagelog.</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/f7fced579e36b72b57e14768026467e4c4511a40"><code>f7fced5</code></a>
Add support for LCOV output</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/1211d3134bb74abb7b00c3c2209091aaab440417"><code>1211d31</code></a>
Fix flake8 error</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/b077753f5d9d200815fe500d0ef23e306784e65b"><code>b077753</code></a>
Use modern approach to specify hook options</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/00713b3fec90cb8c98a9e4bfb3212e574c08e67b"><code>00713b3</code></a>
removed incorrect docs on <code>data_file</code>.</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/b3dda36fddd3ca75689bb3645cd320aa8392aaf3"><code>b3dda36</code></a>
Improve workflow with a collecting status check. (<a
href="https://github-redirect.dependabot.com/pytest-dev/pytest-cov/issues/548">#548</a>)</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/218419f665229d61356f1eea3ddc8e18aa21f87c"><code>218419f</code></a>
Prevent undesirable new lines to be displayed when report is
disabled</li>
<li><a
href="https://github.com/pytest-dev/pytest-cov/commit/60b73ec673c60942a3cf052ee8a1fdc442840558"><code>60b73ec</code></a>
migrate build command from distutils to setuptools</li>
<li>Additional commits viewable in <a
href="https://github.com/pytest-dev/pytest-cov/compare/v2.12.0...v4.0.0">compare
view</a></li>
</ul>
</details>
<br />


Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)


</details>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.

None yet

3 participants