Adding options to log addition additional metadata into the lockfile #204

NatPRoach · 2022-06-30T17:24:27Z

Hello! thank you for making conda-lock, it's a great tool.

This PR would add the ability to specify extra comment lines into the lockfile as well as info about git repository etc. I added these metadata lines as a series of new flags:

    --metadata-jsons
    --metadata-yamls
    --metadata {"timestamp" "git_sha" "git_user_name" "git_user_email" "input_md5" "input_sha"}

--metadata can be specified multiple times to request any of the individual metadata tags, as requested here:
#204 (comment)

…file

netlify · 2022-06-30T17:24:34Z

✅ Deploy Preview for conda-lock ready!

Name	Link
🔨 Latest commit	`16e32c8`
🔍 Latest deploy log	https://app.netlify.com/sites/conda-lock/deploys/6377aa7aab825b000b655f24
😎 Deploy Preview	https://deploy-preview-204--conda-lock.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

mariusvniekerk · 2022-07-01T12:32:55Z

Yeah having the metadata exist as a nice structured pydantic model with the various fields being Optional sounds great to me. When converting the model to raw python dictionary you can easily just exclude the fields with None values

NatPRoach · 2022-07-05T22:10:36Z

Not sure what to do about these timeouts in the tests, but it seems like the ubuntu test that failed in f904e79 passed in 90875c3, and the mac os test that failed in 90875c3 passed in f904e79, despite there being no diffs between those commits. Happy to modify the timeout limit if that's what you'd recommend.

mariusvniekerk · 2022-07-09T01:22:24Z

Can you add some tests to assert that the kind of metadata output we would want remains in the files. These kinds of regression tests are invaluable to ensure that conda-lock has minimal regressions over time.

NatPRoach · 2022-07-11T22:37:34Z

Absolutely. Just wanted to make sure that the approach looked reasonable to you before moving forward on writing tests. I'll try to get that done ASAP.

NatPRoach · 2022-07-13T20:02:09Z

I'm not really sure what's going on with the test that's failing. I didn't change anything related to the code for updating dependencies, and that test is failing for me locally when run on main as well.

NatPRoach · 2022-07-25T13:37:00Z

@mariusvniekerk I think this is passing all CI except the test that seems to be failing on main / other branches as well. Happy to add more tests if you'd like, but if you wouldn't mind taking another look I'd really appreciate it.

mariusvniekerk · 2022-07-28T23:55:52Z

I landed #226 which should hopefully make the test suite here a bit more reliable

mariusvniekerk · 2022-07-29T01:17:09Z

@wolfv Would this change impact micromamba in any way?

wolfv · 2022-07-29T06:57:27Z

Let's ask @Klaim who implemented the lock-file handling in micromamba!

jezdez

Just some nits with the formats and using md5 alone for the inputs metadata.

jezdez · 2022-07-29T08:39:29Z

conda_lock/src_parser/__init__.py

+    def create(cls) -> "TimeMeta":
+        import time
+
+        return cls(created_at=f"{time.asctime(time.gmtime(time.time()))}")


Could you use an ISO 8601 formatted value here instead of asctime which is awkward to parse programmatically?

conda_lock/src_parser/__init__.py

Klaim · 2022-07-29T09:58:32Z

Hello!

@wolfv Would this change impact micromamba in any way?

New fields in "metadata" will be ignored by micromamba (assuming I didn't miss a bug), see details of the parsing in that function. It will only look for specific fields it needs.

mariusvniekerk · 2022-07-29T12:49:33Z

tests/test-input-metadata/conda-lock.lock

+    osx-64: d01c1f5433f30bdbcd3bdad8d9b096774ab55f1210c094acdc61a35b32b28d67
+    win-64: 310b23581083bfb983927c40d3bdc86162192d7b26ffd7bffc385f627c155697
+  inputs_metadata:
+    /Users/c13371nroach/Documents/repos/conda-lock/tests/zlib/environment.yml:


This should probably be a relative path not an absolute one

I think (and I will x2 check this) That it uses a relative path if provided a relative path when conda-lock is invoked, and an absolute one if passed an absolute path. Happy to make the path relative (perhaps to the lockfile?) if you think that's always preferable.

I think that is generally preferable. Leaking in usernames etc into the lockfiles seems undesirable

conda_lock/src_parser/__init__.py

conda_lock/conda_lock.py

conda_lock/src_parser/__init__.py

Co-authored-by: Marius van Niekerk <marius.v.niekerk@gmail.com>

conda_lock/src_parser/__init__.py

Co-authored-by: Jannis Leidel <jannis@leidel.info>

…ig file

NatPRoach · 2022-11-12T21:20:51Z

@mariusvniekerk @maresb
I believe I've addressed the remaining comments you had. Sorry for the delay on that. Please let me know if there's anything else you'd like to see to get this merged.

mariusvniekerk · 2022-11-15T16:12:39Z

Thanks! Let me review this quickly

mariusvniekerk · 2022-11-15T16:50:04Z

We probably don't need --metadata-json and --metadata-yaml as json is a subset of yaml

maresb · 2022-11-15T19:47:13Z

conda_lock/src_parser/__init__.py

+        default=None,
+        description="Metadata dealing with the input files used to create the lockfile",
+    )
+    custom_metadata: Optional[Dict[str, str]] = Field(


Suppose I want to include something like environment.yml which contains lists. Shall we make this a fully general dict?

(Needs from typing import Any)

Suggested change

custom_metadata: Optional[Dict[str, str]] = Field(

custom_metadata: Optional[Dict[Any, Any]] = Field(

Ideally not. It places a lot of extra strain on other implemations that make use of the file format like micromamba. Any is FAR too broad a type. I can potentially see value in a Union[str, list[str]]

Good point. Ideally it would be some sort of recursive type like T: Union[int, str, list[T], dict[str,T]], but Pydantic chokes on that already.

Note how requirements must be at least list[Union[str, dict[str, list[str]]]] due to ["pip", {"pip": ["pypi-dep"]}]. Thus to parse most requirements.txt we'd need at least something like

dict[str, Union[str, list[Union[str, dict[str, list[str]]]]]]

(assuming I didn't make any mistakes).

Given the complexity, I've convinced myself that this should be a separate feature. But in this case, we should probably make it clear in the --help that metadata-jsons/metadata-yamls contents must be dict[str,str].

Do we want to support dict[str, Union[str, int, float]]?

What I'm getting at here is, if we have

# md.yaml x: a y: 1 z: 1.5

is it correct to have the current result of conda-lock --metadata-yaml md.yaml be as follows?

custom_metadata: x: a y: '1' z: '1.5'

Perhaps that's the simplest since strings faithfully encode most reasonable types, and it prevents other potential headaches.

conda_lock/conda_lock.py

Co-authored-by: Ben Mares <services-git-throwaway1@tensorial.com>

maresb · 2022-11-18T16:13:02Z

In the near future someone may propose a command-line option to add a key-value pair to the custom metadata.

Perhaps it should be called something like --add-metadata which could be used like

conda-lock --metadata git_sha --add-metadata short_git_sha=$(git rev-parse --short HEAD)

Are we satisfied with using --metadata to select these pre-defined fields defined in this PR? Or would it be better to rename --metadata to something more specific like --metadata-opt?

mariusvniekerk · 2022-11-22T18:30:43Z

probably sufficient as the first draft. I want to make some tweaks to the models a little bit in a subsequent pr. And we can get the cli cleaned up in another one.

* Fixes conda#229, relying on what introduced in conda#204 for the input metadata, by explicitly handling only the case of different drives for keeping absolute paths, addressing conda#175.

* Fixes #229, relying on what introduced in #204 for the input metadata, by explicitly handling only the case of different drives for keeping absolute paths, addressing #175.

Adding options to log metadata about lockfile in comments to the lock…

d9b34a7

…file

NatPRoach changed the title ~~Adding options to log metadata about lockfile in comments to the lockfile~~ Adding options to log metadata about lockfile in comments in the lockfile Jun 30, 2022

Address failing lints

cfda7bd

NatPRoach marked this pull request as draft June 30, 2022 18:29

Nathan Roach added 4 commits July 5, 2022 11:38

Structuring metadata as pydantic models on the LockMeta class

604b9fe

Restructuring pydantic a bit to have less redundancy

f448f05

Fixing new lints

90875c3

trying to kickoff tests again after timeout last time

f904e79

NatPRoach marked this pull request as ready for review July 5, 2022 22:10

Adding regression tests for new metadata fields

e7c613c

NatPRoach force-pushed the add_lockfile_metadata branch from 70173a3 to e7c613c Compare July 14, 2022 16:03

Merge branch 'main' into add_lockfile_metadata

dd97f51

Merge branch 'main' into add_lockfile_metadata

811662b

jezdez reviewed Jul 29, 2022

View reviewed changes

mariusvniekerk requested changes Jul 29, 2022

View reviewed changes

mariusvniekerk reviewed Jul 29, 2022

View reviewed changes

conda_lock/src_parser/__init__.py Outdated Show resolved Hide resolved

Update conda_lock/src_parser/__init__.py

8f4c6af

Co-authored-by: Marius van Niekerk <marius.v.niekerk@gmail.com>

jezdez reviewed Jul 29, 2022

View reviewed changes

conda_lock/src_parser/__init__.py Outdated Show resolved Hide resolved

Update conda_lock/src_parser/__init__.py

fe54b6b

Co-authored-by: Jannis Leidel <jannis@leidel.info>

NatPRoach added 9 commits November 12, 2022 11:10

Removing unused and duplicate click fields that got readded in merge

107508f

fixing path resolution when files are on different drives

d871556

reworking git metadata check to work in abscence of pre-existing conf…

ef3cd98

…ig file

Fixing precommit lints

f5761de

attempting to fix git metadata issue

a833d13

Move to the git directory to address the relative path issue

4bee557

address user section not existing on github actions

da86fac

Fixing mistype on addressing user section not existing in github actions

ae256e3

Addressing releasing lock of config writer

941f0d8

NatPRoach changed the title ~~Adding options to log metadata about lockfile in comments in the lockfile~~ Adding options to log addition additional metadata into the lockfile Nov 12, 2022

NatPRoach requested review from mariusvniekerk and removed request for jezdez November 12, 2022 21:28

mariusvniekerk added 2 commits November 15, 2022 12:49

Cleanup type annotations a bit

1e910d6

Fix potential pydantic pyright type inference issues

645512d

maresb reviewed Nov 15, 2022

View reviewed changes

conda_lock/conda_lock.py Outdated Show resolved Hide resolved

mariusvniekerk and others added 3 commits November 15, 2022 16:34

Fix windows skip

19199f1

Remove some unused imports

9a5292a

Update conda_lock/conda_lock.py

16e32c8

Co-authored-by: Ben Mares <services-git-throwaway1@tensorial.com>

mariusvniekerk approved these changes Nov 22, 2022

View reviewed changes

mariusvniekerk merged commit e1feddd into conda:main Nov 22, 2022

bollwyvl mentioned this pull request Dec 14, 2022

Make gitpython dependency optional? #296

Open

riccardoporreca mentioned this pull request Feb 4, 2023

Store lockfile-relative sources in lockfiles #328

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding options to log addition additional metadata into the lockfile #204

Adding options to log addition additional metadata into the lockfile #204

NatPRoach commented Jun 30, 2022 •

edited

Loading

netlify bot commented Jun 30, 2022 •

edited

Loading

mariusvniekerk commented Jul 1, 2022

NatPRoach commented Jul 5, 2022

mariusvniekerk commented Jul 9, 2022

NatPRoach commented Jul 11, 2022

NatPRoach commented Jul 13, 2022

NatPRoach commented Jul 25, 2022

mariusvniekerk commented Jul 28, 2022

mariusvniekerk commented Jul 29, 2022

wolfv commented Jul 29, 2022

jezdez left a comment

jezdez Jul 29, 2022

Klaim commented Jul 29, 2022 •

edited

Loading

mariusvniekerk Jul 29, 2022

NatPRoach Jul 29, 2022

mariusvniekerk Jul 29, 2022

NatPRoach commented Nov 12, 2022 •

edited

Loading

mariusvniekerk commented Nov 15, 2022

mariusvniekerk commented Nov 15, 2022

maresb Nov 15, 2022

mariusvniekerk Nov 15, 2022

maresb Nov 15, 2022

maresb Nov 15, 2022

maresb Nov 15, 2022

maresb commented Nov 18, 2022 •

edited

Loading

mariusvniekerk commented Nov 22, 2022

	custom_metadata: Optional[Dict[str, str]] = Field(
	custom_metadata: Optional[Dict[Any, Any]] = Field(

Adding options to log addition additional metadata into the lockfile #204

Adding options to log addition additional metadata into the lockfile #204

Conversation

NatPRoach commented Jun 30, 2022 • edited Loading

netlify bot commented Jun 30, 2022 • edited Loading

✅ Deploy Preview for conda-lock ready!

mariusvniekerk commented Jul 1, 2022

NatPRoach commented Jul 5, 2022

mariusvniekerk commented Jul 9, 2022

NatPRoach commented Jul 11, 2022

NatPRoach commented Jul 13, 2022

NatPRoach commented Jul 25, 2022

mariusvniekerk commented Jul 28, 2022

mariusvniekerk commented Jul 29, 2022

wolfv commented Jul 29, 2022

jezdez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Klaim commented Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NatPRoach commented Nov 12, 2022 • edited Loading

mariusvniekerk commented Nov 15, 2022

mariusvniekerk commented Nov 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maresb commented Nov 18, 2022 • edited Loading

mariusvniekerk commented Nov 22, 2022

NatPRoach commented Jun 30, 2022 •

edited

Loading

netlify bot commented Jun 30, 2022 •

edited

Loading

Klaim commented Jul 29, 2022 •

edited

Loading

NatPRoach commented Nov 12, 2022 •

edited

Loading

maresb commented Nov 18, 2022 •

edited

Loading