🚸 Enable linting scripts to check whether they produce complete data lineage - #3846
🚸 Enable linting scripts to check whether they produce complete data lineage#3846sheetalgiri wants to merge 37 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3846 +/- ##
==========================================
- Coverage 91.55% 87.04% -4.52%
==========================================
Files 88 89 +1
Lines 15747 16066 +319
==========================================
- Hits 14417 13984 -433
- Misses 1330 2082 +752
🚀 New features to boost your workflow:
|
|
Deployment URL: https://6a5bda96.lamindb.pages.dev |
…d checks: - lineage tracking calls (ln.track(...) / ln.finish(...) or imported equivalents) - LaminDB input retrieval patterns (e.g. ln.Artifact.get(...), ln.Collection.filter(...), etc.) - LaminDB output persistence patterns (.save() / ln.save(...)) - suspicious non-LaminDB input reads (e.g. pd.read_*, open, np.load, etc.)
- Instead of having checks like has_lamindb_inputs and has_lamindb_outputs, use checks has_external_inputs and has_external_outputs. in this case even if there is no input/output a script can be correct - if there are external inputs/outputs printing those function calls, to help guide the agent in what it should fix
lamin-cli: points to main (#254 merged). lamin-skills: points to the latest commit on sheetalgiri-patch-1 (includes the --session-id finish fix on top of verify_lineage).
lamin-cli: points to main (#255 merged). lamin-skills: points to the latest commit on the still-open sheetalgiri-patch-1 branch.
- checks for file names in a script like 'x.txt'or 's.npy'or anything and whether or not they are associated with a lamin call or not , regardless of what it is, directly or indirectly, if all file names have this kind of association the function passes the script
| `ln.track()` and `ln.finish()`. | ||
| - Extracts path-like strings from literals, variables, call arguments, | ||
| and simple path joins (string `+` or path `/` operations). | ||
| - Classifies path usage as tracked when it appears in LaminDB calls |
There was a problem hiding this comment.
I see quite a lot of logic related to this. Why not just flag paths that are not going through lamindb? Wouldn't this enable us to eliminate a lot of the code?
There was a problem hiding this comment.
This was my initial idea but in practice I saw agents using non-lamindb write functions in combination with lamindb save. Its also inevitable in general because we would have to do this for file formats other than those where we have ln.Artifact.from_* methods like from_dataframe or from_anndata
| and simple path joins (string `+` or path `/` operations). | ||
| - Classifies path usage as tracked when it appears in LaminDB calls | ||
| (`ln.*`, `lamindb.*`, including `ln.Artifact(...).save()` patterns), | ||
| and flags paths that only appear in non-LaminDB calls. |
| assert result.missing_lineage == () | ||
|
|
||
|
|
||
| def test_verify_lineage_positive_output_from_dataframe(tmp_path: Path): |
There was a problem hiding this comment.
I would combine this test and the previous one through parametrization. Both tests are very simple baseline tests.
| assert result.missing_lineage == () | ||
|
|
||
|
|
||
| def test_verify_lineage_imported_symbols_match_current_behavior(tmp_path: Path): |
There was a problem hiding this comment.
What is the reason for testing this false positive behavior? 🤔
There was a problem hiding this comment.
this is a mistake, it should be tracked
| assert "Missing ln.finish() call in script." in result.missing_lineage | ||
|
|
||
|
|
||
| def test_verify_lineage_positive_zero_io_script(tmp_path: Path): |
There was a problem hiding this comment.
I would merge this into tests 1 & 2 through parametrization as the structure is the same
| assert result.missing_lineage == () | ||
|
|
||
|
|
||
| def test_verify_lineage_negative_missing_lineage_tracking_calls(tmp_path: Path): |
There was a problem hiding this comment.
What about @ln.flow()? If I had a script that used @ln.flow() instead of ln.track() and ln.finish(), would that be also a false negative (meaning negative is "not tracked").
| assert "Missing ln.finish() call in script." in result.missing_lineage | ||
|
|
||
|
|
||
| def test_verify_lineage_negative_external_input_even_when_script_has_lamindb_io(tmp_path: Path): |
There was a problem hiding this comment.
Great test! I think creating a catalog of these negative examples and then running a parametrized test against them is the most important
What matters is to detect scripts that don't produce lineage with high confidence
| assert any("./local_input.csv" in item for item in result.missing_lineage) | ||
|
|
||
|
|
||
| def test_verify_lineage_negative_external_output_write(tmp_path: Path): |
| assert any("./local_output.txt" in item for item in result.missing_lineage) | ||
|
|
||
|
|
||
| def test_verify_lineage_negative_open_read_untracked(tmp_path: Path): |
| assert any("./local_input.txt" in item for item in result.missing_lineage) | ||
|
|
||
|
|
||
| def test_verify_lineage_positive_local_write_then_lamindb_save(tmp_path: Path): |
There was a problem hiding this comment.
That's cool that this is properly classified. Impressive!
|
Cool! 😄 (1) What about scripts that track lineage via |
|
@falexwolf thanks for the thorough feedback !
This is something I missed, will add it
Exactly, when the path is written to a temp file it adds it to the 'untracked' list. Once it sees that the same path is saved on Lamindb, it adds it to the 'tracked' list and then its considered resolved.
Makes sense 👍
I would say we use it internally first and get more feedback/experience using it with agents, what do you think? |
Agreed! |
|
@falexwolf I made the changes, could you please have another look? |
|
While testing this, I noticed that our skills are kind of bad at using import lamindb as ln
# 1. Start global script tracking
ln.track()
# 2. Use @ln.flow() for functions nested inside an ln.track() session
@ln.flow()
def process_sample(sample_id: str):
df = ln.examples.datasets.mini_immuno.get_dataset1()
return ln.Artifact.from_dataframe(df, key=f"processed_{sample_id}.parquet").save()
# 3. Call the step function
artifact = process_sample(sample_id="batch_01")
# 4. Finish the global tracking session
ln.finish()which caused the error RuntimeError: Please use @ln.step() or clear the global run context before using @ln.flow(): no `ln.track()` or `@ln.flow(global_run='clear')`So, ln.flow() should never be used inside ln.track(). Is that correct @falexwolf ? I could add the skills for this but I would need to understand better when exactly we want agents to use ln.flow(), is it something meant to be used for notebooks and not for normal scripts? FYI @Ebad371 |
Yes, that's correct. While |
Added verify_lineage() function that parses a Python script with
astand checks:Updated skills.md so that for every python script that the agent creates/plans to run, it verifies the lineage by calling the function and doing assertions on the verification results, make sure everything passes. If there is a failure, it goes back and makes modifications to the script and runs the function again.
Update skills.md to create artifacts from in-memory objects when possible using from_dataframe(), from_anndata(),etc instead of saving it to disk and doing save()
Why:
Limitations: