Skip to content

Cleanup of em_data in prep for more changes - #13

Open
arthurmccray wants to merge 4 commits into
CSSFrancis:mainfrom
arthurmccray:cleanup
Open

arthurmccray wants to merge 4 commits into
CSSFrancis:mainfrom
arthurmccray:cleanup

Conversation

@arthurmccray

Copy link
Copy Markdown

Correctness fixes, typing, and lint/type-check tooling

Four commits: tooling, a mechanical lint pass, then two rounds of fixes. The linting
is really the main stuff here.

DownloadFuture is renamed DatasetPath, and download() now returns one in
both modes, instead of a str when blocking and a path subclass when not.

Bug fixes

Fixing DatasetPath pending status: the pending future was stored on the
instance, but pathlib builds a new object for any derived path
(handle.parent / handle.name, handle.with_suffix(...)). The copy had no
future, and both __fspath__ and done read it with getattr(..., None),
which treats a missing future as finished. A derived path therefore reported
done == True during an active download, and did not wait when passed to a
reader. Pending downloads now live in a module-level dict keyed by path, so any
path naming that file waits.

Distinguishing shared from user copies: filepath() searches the shared
dirs before the user dir, but delete() only looks in the user dir. That is the
behaviour we want, but the widget called delete(), discarded the result and
redrew, so on a shared install the button appeared to do nothing. Catalogue
entries now carry location ("user", "shared" or None), and both widgets
show a different badge and no delete button for a shared copy. delete() itself
is unchanged.

Fixing subclassing: the generated __init__ used super(self.__class__, self), which resolves against the runtime class and only works when self is
exactly that class, so subclassing any dataset raised a TypeError. Classes now
hold their YAML entry as _spec and the base __init__ reads it. There is no
generated __init__ any more.

Tooling

  • [tool.ruff] (line length 99, E4, E7, E9, F, I) and [tool.basedpyright]
    (standard), a dev extra, and a pre-commit config.
  • A lint job in CI running both, separate from the test matrix. Both are
    blocking: the package is at 0 ruff errors and 0 basedpyright errors.
  • One ruff --fix + ruff format commit, kept separate from the behaviour
    changes. Markdown is excluded so the README is not reformatted.
  • The generated .pyi ended with __all__ = __all__ + [...], which references a
    name a stub never binds. It is a plain list now.

Breaking changes

Worth a minor version bump.

Was Now
download() -> str or a path subclass, depending on background always DatasetPath (a pathlib.Path subclass)
filepath() -> str | None Path | None
result() -> str Path
DownloadFuture DatasetPath
handle.done() handle.done (property)
config.data_dir(), shared_data_dirs(), data_search_dirs() -> str Path
em_database.get_data_dir() -> str Path
DownloadableDataset(source, file, ...) DownloadableDataset(**overrides), reading _spec

On the two renames: DownloadFuture did not implement the Future interface
(no cancel(), exception() or add_done_callback()), and its _future
attribute held an actual concurrent.futures.Future. done is a property
because as a method if path.done: is always true, while path.done() on a
property raises straight away.

Settings values are still stored as strings, so existing
~/.em_database/settings.yaml files are unaffected.

Verification

93 tests pass (7 new), 0 ruff errors, 0 basedpyright errors at standard.

@arthurmccray

Copy link
Copy Markdown
Author

Including here the next things to change so we can discuss them (here or on slack)

Next PR: the metadata model (breaking)

This rewrites every dataset YAML and the public attribute surface.

Was Becomes
data_size: "1.4 GB" (str) size_bytes: 1503238553 (int), with ds.size for formatted display
ds.detector works, ds.technique raises everything descriptive in metadata, reachable as attributes
microscope_vendor enum enforced in the schema open string, with a CI check that warns

Non-breaking, same PR: validate the dataset YAML against the shipped JSON
schema (nothing does today, in CI or at import), and a CI check that the
generated .pyi is current.

The vendor check separates a typo from a new vendor by fuzzy match:
Direct electron fails, NORDIF warns and asks you to add it to the list.

After that: search (additive)

em_database.search() / filter() / datasets() returning dataset objects.
Today only the widget can filter the catalogue. The widget's own search box gets
pointed at the same function.

Later: config (breaking, at integration)

Replace the bespoke Settings object with the dask-style config quantem uses:
em_database.settings and its save() / reset() go away, and the settings
widget is rewritten. Held until the package moves under electronmicroscopy,
since that is also when the naming and the data directory default change.

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.

1 participant