Skip to content
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

Merged
merged 32 commits into from
May 23, 2024
Merged

Project init #1

merged 32 commits into from
May 23, 2024

Conversation

f-PLT
Copy link
Collaborator

@f-PLT f-PLT commented Apr 26, 2024

Project structure and tools initialization

@f-PLT f-PLT self-assigned this Apr 26, 2024
@f-PLT
Copy link
Collaborator Author

f-PLT commented Apr 26, 2024

Not quite done yet, but it's enough to start talking about it!

@f-PLT f-PLT requested a review from liellnima April 26, 2024 22:27
@f-PLT f-PLT marked this pull request as ready for review May 13, 2024 21:38
@f-PLT f-PLT requested a review from mjfortier May 13, 2024 21:39
@f-PLT
Copy link
Collaborator Author

f-PLT commented May 13, 2024

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.

Copy link
Collaborator

@liellnima liellnima left a 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-create-env

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_conda-poetry-install

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.

make_test

After those things are fixed, I can approve the PR :)

@f-PLT
Copy link
Collaborator Author

f-PLT commented May 17, 2024

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

(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-create-env

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_conda-poetry-install

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.

make_test

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

@f-PLT
Copy link
Collaborator Author

f-PLT commented May 17, 2024

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

This is strange, I cannot reproduce this error... (double checked that I did have conda as the CONDA_TOOL)

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..]).

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 environment.yml file. I'll push a modification and we'll see if it works on your end.

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.

This is expected behavior.

@liellnima
Copy link
Collaborator

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.

@f-PLT
Copy link
Collaborator Author

f-PLT commented May 22, 2024

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 pix installation, as using it on the cluster requires using a virtual environment, because packages can't be installed with the system's pip (no write permissions)

@f-PLT f-PLT requested a review from liellnima May 22, 2024 17:22
@liellnima
Copy link
Collaborator

liellnima commented May 23, 2024

Nice! Everything runs smoothly on my local machine!

Info

  • make targets
  • make help
  • make version

Conda targets

  • conda-create-env
  • conda-env-info
  • conda-poetry-install
  • conda-poetry-uninstall
  • conda-clean-env

Poetry targets

  • make poetry-install-auto
  • make poetry-install
  • make poetry-install-venv
  • make poetry-env-info
  • make poetry-create-env
  • make poetry-remove-env
  • make poetry-uninstall-venv

Install targets

  • make install
  • make install-with-lab

Linting targets

  • make check-lint
  • make check-pylint
  • make fix-lint
  • make precommit

Test targets

  • make test
  • make test-marker
  • make test-specific
  • make test-custom

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.

Copy link
Collaborator

@liellnima liellnima left a 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.

Copy link
Collaborator

@mjfortier mjfortier left a 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.

@f-PLT f-PLT merged commit d763a76 into main May 23, 2024
4 checks passed
@f-PLT f-PLT deleted the project-init branch May 28, 2024 13:33
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.

3 participants