Skip to content

Now handling all constraint notations in unpinned enviroment.yamls#860

Merged
vringar merged 3 commits into
masterfrom
prune_env
Feb 23, 2021
Merged

Now handling all constraint notations in unpinned enviroment.yamls#860
vringar merged 3 commits into
masterfrom
prune_env

Conversation

@vringar

@vringar vringar commented Feb 16, 2021

Copy link
Copy Markdown
Contributor

@birdsarah mentioned in #858 (comment) that we don't pin dependencies specified by < or >.

This PR fixes that inconsistency and guarantees that the last not_pip dependency will get pinned if there are no pip dependencies

@birdsarah birdsarah left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks fine from a cursory look. If repin works without changing the pinned env file with the new < then I'd say we're good to go.

What this is highlighting for me is how gross this is.

I built it to match the infrastructure we were replacing. I can't remember the logic for wanting a pruned environment file.

We could switch to this kind of thing which would let us have a similar setup but without this prune step which just feels like a maintenance headache.

Comment thread scripts/prune-environment.py
@vringar

vringar commented Feb 16, 2021

Copy link
Copy Markdown
Contributor Author

I can't remember either why we didn't want full pinning
Paging @englehardt for context

@vringar

vringar commented Feb 17, 2021

Copy link
Copy Markdown
Contributor Author

This looks fine from a cursory look. If repin works without changing the pinned env file with the new < then I'd say we're good to go.

I ran ./repin.sh and nothing changed.

We could switch to this kind of thing which would let us have a similar setup but without this prune step which just feels like a maintenance headache.

These conda environments seem to be specific to the OS and Processor used. So I'm hesitant to use this.
I think we can just not do the dependency pruning after running conda env export

Comment thread scripts/prune-environment.py
Comment thread scripts/prune-environment.py Outdated
@birdsarah

Copy link
Copy Markdown
Contributor

This looks fine from a cursory look. If repin works without changing the pinned env file with the new < then I'd say we're good to go.

I ran ./repin.sh and nothing changed.

We could switch to this kind of thing which would let us have a similar setup but without this prune step which just feels like a maintenance headache.

These conda environments seem to be specific to the OS and Processor used. So I'm hesitant to use this.
I think we can just not do the dependency pruning after running conda env export

YES! This is exactly what I ran into before. Thanks for jogging my memory.

@englehardt

Copy link
Copy Markdown
Collaborator

I can't remember either why we didn't want full pinning
Paging @englehardt for context

I prefer full pinning. IIRC not pruning would lead to issues. Maybe @birdsarah can speak to the motivation for implementing pruning?

@vringar

vringar commented Feb 22, 2021

Copy link
Copy Markdown
Contributor Author

Seeing as she said:

I built it to match the infrastructure we were replacing. I can't remember the logic for wanting a pruned environment file.

I don't think she can speak to why the pruning was implemented.
Do we just want to drop the pruning and see if we encounter any issues? Or should I try to find the PR and the associated issue for introducing conda and try to figure out why we were pruning?

@englehardt

Copy link
Copy Markdown
Collaborator

Seeing as she said:

I built it to match the infrastructure we were replacing. I can't remember the logic for wanting a pruned environment file.

I don't think she can speak to why the pruning was implemented.
Do we just want to drop the pruning and see if we encounter any issues? Or should I try to find the PR and the associated issue for introducing conda and try to figure out why we were pruning?

Check out the May 14th commits here: #648

It sounds like it might have been OS differences in dependencies? As in maybe plyvel requires one dependency on Linux and a different one on MacOS.

@vringar

vringar commented Feb 22, 2021

Copy link
Copy Markdown
Contributor Author

Also dependencies on XServer are probably not satisfiable under MacOS

@codecov

codecov Bot commented Feb 22, 2021

Copy link
Copy Markdown

Codecov Report

Merging #860 (1db7bc0) into master (9aaa3c0) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #860   +/-   ##
=======================================
  Coverage   49.66%   49.66%           
=======================================
  Files          34       34           
  Lines        3306     3306           
=======================================
  Hits         1642     1642           
  Misses       1664     1664           

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 9aaa3c0...1db7bc0. Read the comment docs.

@boolean5 boolean5 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is ready to merge, right?

@vringar

vringar commented Feb 23, 2021

Copy link
Copy Markdown
Contributor Author

Yes

@vringar vringar merged commit cb95ecc into master Feb 23, 2021
@vringar vringar deleted the prune_env branch February 23, 2021 10:36
Zaxeli pushed a commit to Zaxeli/OpenWPM that referenced this pull request Aug 10, 2021
…penwpm#860)

* Now handling all constraint notations in unpinned enviroment.yamls

* Addressing review comments
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.

4 participants