Skip to content

Allow for export and input of knowls via yaml - #7029

Merged
jenpaulhus merged 6 commits into
LMFDB:mainfrom
roed314:knowl_yaml
Aug 3, 2026
Merged

Allow for export and input of knowls via yaml#7029
jenpaulhus merged 6 commits into
LMFDB:mainfrom
roed314:knowl_yaml

Conversation

@roed314

@roed314 roed314 commented Jun 4, 2026

Copy link
Copy Markdown
Member

No description provided.

@roed314 roed314 added the knowls Displaying or editing knowls (not content related) label Jun 4, 2026
@jenpaulhus

Copy link
Copy Markdown
Contributor

Is this ready to be reviewed?

@roed314

roed314 commented Jul 15, 2026

Copy link
Copy Markdown
Member Author

Yep, this is ready to be reviewed! The goal is to allow for changes to many knowls at once by exporting the current state, modifying on disk, then uploading.

@jenpaulhus

jenpaulhus commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

I asked Claude to take a look at this PR too and to test it for correctness/safety/bugs. It had one tiny bug and a few small suggestions (see below). Several of them are cautions for careless usage which we may not want to worry about. For (3) in Correctness, I'm not sure what we want the general behavior to be. If this is just fixing a small typo, they should not go back to being unreviewed. But if it is something more substantial, maybe?

And in the smaller issue, I do think we want to not add the author name by default to all of these knowls if the person doing the import/export is just fixing a small issue.


Correctness

1. allowed_id() will crash on an invalid id (real bug)

In the creating branch of yaml_import:

from lmfdb.knowledge.main import allowed_id
...
if not allowed_id(kid):
    print(f"{kid} not a valid knowl id")
    continue

allowed_id (lmfdb/knowledge/main.py:44) calls flash_error for a bad id, and flash_error (lmfdb/utils/utilities.py:781) calls Flask's flash(), which requires a request context. But the docstring says yaml_import "must be run manually by an LMFDB editor" — i.e., from a console with no request context.

So a valid id returns cleanly, but an invalid id raises RuntimeError: Working outside of request context instead of printing the message and continuing — exactly the case the guard is meant to handle gracefully.

Suggestion: use a context-free validation helper (or wrap the call in a try/except), rather than reusing the web-facing allowed_id.

2. The save refactor is correct (verified)

Moving the Knowl → {'authors': ...} conversion out of Knowl.save and into KnowlBackend.save via isinstance(most_recent, Knowl) handles all existing callers:

  • The dict-passing callers at knowl.py:373 / :398 still hit the .get('authors', []) path.
  • The Knowl-passing rename caller at knowl.py:636 is now normalized in the backend.

The switch from .pop to .get is safe since most_recent isn't reused afterward. No regression.

3. straight_to_production=True keeps the old status even when content changed

For an update it doesn't reset status to 0, so a knowl that was status=1 (reviewed) retains "reviewed" status with new, unreviewed content. This may be intentional for trusted admin edits, but it's a foot-gun worth a docstring warning. (New knowls are always forced to status 0, which is a slight inconsistency but the safer default.)


Smaller Issues

  • Docstring bug: yaml_export's docstring says to reload "using update_from_yaml", but the method is named yaml_import. Also the sentence "records the timestamp that this" is cut off.
  • Author attribution: updates always add who to the authors list (no minor=True path exposed), so whoever runs a bulk import becomes a co-author on every touched knowl. Minor, but a flag may be desirable.
  • YAML round-trip fidelity: knowl content is multi-line markdown. YAML block scalars normalize trailing whitespace and require a final newline, so an export→import with no edits could occasionally produce a spurious diff. Low risk, but worth a one-time round-trip check on real data.
  • No overall transaction: each save commits independently, so a failure partway through leaves a partial import. Since edits land on beta and are reviewable, it's recoverable, but wrapping the loop in a single DelayCommit would be cleaner.
  • Timestamp comparison (rec["timestamp"] >= timestamp) assumes both sides are naive datetimes. That matches the codebase's utc_now_naive / ensure_naive_utc convention, and PyYAML round-trips naive datetimes as naive, so it should be fine — worth confirming on real data once.
  • ids=0 default is a bit magic (0 = the "normal" knowl type code). A named default or a short comment would help future readers.

- Fix crash on invalid knowl ids in yaml_import: allowed_id calls
  flash_error, which requires a Flask request context, but yaml_import
  is run from the command line.  The validation logic now lives in a
  context-free helper bad_knowl_id_reason in knowl.py (also fixing a
  latent formatting crash when a bad id contains a percent sign), and
  invalid comment ids now flash an explanatory message on the web
  instead of failing silently.
- Wrap the import loop in a single DelayCommit so a failure partway
  through does not leave a partial import.
- Add a minor flag to yaml_import (default True) so the person running
  a bulk import is not added as an author of every touched knowl;
  creators of new knowls are still recorded.
- Document the straight_to_production status foot-gun, fix the
  yaml_export docstring (update_from_yaml -> yaml_import, truncated
  sentence, wrong fields default) and explain the ids=0 default.

Tested against devmirror (dryrun only): a full export of all 1722
normal knowls re-imports with zero spurious diffs, so the yaml round
trip is faithful on real data; invalid ids in a creation batch now
print a message instead of raising RuntimeError.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Address review feedback on yaml knowl import/export
@roed314

roed314 commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

@jenpaulhus I think I've addressed all of the comments.

@jenpaulhus

Copy link
Copy Markdown
Contributor

LGTM!

@jenpaulhus
jenpaulhus merged commit e133593 into LMFDB:main Aug 3, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

knowls Displaying or editing knowls (not content related)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants