-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Project init #1
Project init #1
Conversation
Not quite done yet, but it's enough to start talking about it! |
I could probably keep working on this forever, but I think it's far enough along. I added specific parts for environment management, as I thinks it's the easiest place to get lost for newcomers and people unfamiliar with either Conda or Poetry. Let me know what you think, what's lacking and/or hard to comprehend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for putting all of this together!!
Things look great, there are just two minor fixes I would like to request (these issues seem only to pop up for conda users), and one thing where I am not sure if it's the expected behavior.
Step-by-step:
- make targets
- make conda-create-env
- make conda-poetry-install
- make install
- make check-lint
- make precommit
- make test [UPDATE: expected behavior]
(disclaimer: all the other targets have not been tested by me right now)
make conda-create-env
In line 114 of .make/base.make a problem arises: The -y tag is not recognized by conda. To run this in conda I did: conda env create -f environment.yml
make conda-poetry-install
:
In line 122 in .make/base.make a problem arises: The poetry package could not be found (PackagesNotFoundError) via the anaconda channels. However, I could install the package with conda-forge instead: conda install conda-forge::poetry==1.8.2
(works also with conda install -y [..etc..]).
make test
:
The test fails despite the fact there are no tests to run. See screenshot attached. I am not sure if that error message is expected behaviour since there are no tests yet, or if something else is going on here.
After those things are fixed, I can approve the PR :)
Well darn... I was sure I fixed this.. must have crept its way back when I added the possibility of installing to the conda environment without having to activate it. I should have a fix soon enough |
This is strange, I cannot reproduce this error... (double checked that I did have conda as the CONDA_TOOL)
Hmm ok, I get what happened. I usually consider the setting of defaults channels to include conda-forge, but I get that not necessarily every one would. I'll add it explicitly. This is weird though, because conda-forge is defined as a channel in the
This is expected behavior. |
As you have written me on Slack, the first issue is from a too-old conda version, right? (as we also have it on the cluster) And for the second thing, I will just wait until you pushed some changes and then I will retry it :) And the third thing: great, check. |
This is relevant in remote compute cluster environments.
Alright, ready for some more tests to see if I missed anything. I did take the opportunity to add for new targets and documentation for |
Nice! Everything runs smoothly on my local machine! Info
Conda targets
Poetry targets
Install targets
Linting targets
Test targets
I have not tested the versioning targets. The rest seems to work :D Would love to get an introduction in the next meeting how to handle the test-marker / test-specific / test-custom :) I will approve the merge and we can close this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See last comment on the thread. The changes have solved all issues, and the PR is approved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ;)
Appreciate following this back and forth.
Project structure and tools initialization