fix: Implement Automatic Activation of Virtual Environment in the dev nox session #3408

akhilender-bongirwar · 2023-10-04T19:37:25Z

Description

Implement automatic activation of the virtual environment in the 'dev' Nox session
Enhance the 'dev' Nox session defined in noxfile.py
Simplify the development setup process for contributors and developers

This commit enhances the 'dev' Nox session to automatically activate the virtual environment when running 'nox -s dev'.

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

New feature (non-breaking change which adds functionality)
Optimization (back-end change that speeds up the code)
Bug fix (non-breaking change which fixes an issue)

Key checklist:

No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
All tests pass: $ python run-tests.py --all (or $ nox -s tests)
The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

Code is commented, particularly in hard-to-understand areas
Tests added that prove fix is effective or that feature works

…v' Nox Session - Implement automatic activation of the virtual environment in the 'dev' Nox session - Enhance the 'dev' Nox session defined in `noxfile.py` - Simplify the development setup process for contributors and developers This commit enhances the 'dev' Nox session to automatically activate the virtual environment when running 'nox -s dev'. Signed-off-by: Akhilender <[email protected]>

agriyakhetarpal

Thanks, @akhilender-bongirwar, looks off to a good start! I tested this locally and it doesn't look like there is a method to activate the virtual environment from inside a nox session (we should ask users to do so like they currently do). We can target some other relevant improvements, some comments below

noxfile.py

- Imported `Path` directly from `pathlib` for better readability. - Moved the definition of `venv_dir` closer to `PYBAMM_ENV` and marked it as a constant. - Installed development dependencies using `-e .[dev]` to set up the developer environment and silenced the warning with `external=True`. - Added the installation of `[jax,odes]` extras for macOS and Linux platforms. Signed-off-by: Akhilender <[email protected]>

agriyakhetarpal

I think this is almost ready. Could you update the documentation for the source installation in this file, highlighting the change and updating the new location of the virtual environment directory (plus how to activate it)? I left some more comments below

noxfile.py

… enhancement/activate-venv-dev-nox

- Installed `virtualenv` and `cmake` before other dependencies. - Installed all and dev dependencies together using a single `pip` command to avoid redundant wheel building. - Corrected sys.platform to maintain consistency in the code Signed-off-by: Akhilender <[email protected]>

noxfile.py

agriyakhetarpal

You also need to update the documentation for the source installation page as highlighted above

agriyakhetarpal · 2023-10-07T14:36:19Z

You need to update this section. We need to 1. update the location of the virtual environment—should now be venv/ instead of .nox/dev/ wherever it is referenced, and 2. update the command used to activate the virtual environment—this should be source venv/bin/activate instead of .nox/dev/bin/activate, and so on

akhilender-bongirwar · 2023-10-07T14:39:07Z

Thank you @agriyakhetarpal : ).

akhilender-bongirwar · 2023-10-07T15:14:48Z

@agriyakhetarpal, please review the changes : ).

agriyakhetarpal · 2023-10-07T23:05:07Z

The Files interface currently shows that that the entirety of the lines in the two files were modified. Did you change the Git settings about your line endings on your platform recently, or otherwise push the last commit from another platform that might have a different line ending scheme compared to the commits before that? We would have to normalise it to properly display the modified lines. Could you drop your most recent commit and force push? This answer might help: https://stackoverflow.com/a/30757723

akhilender-bongirwar · 2023-10-08T08:29:07Z

@agriyakhetarpal , I actually pushed the commit from WSL instead of using Bash. I reverted the last the last commit.

- Corrected the space between virtualenv and cmake - Changed macos to darwin

- Updated the docs regarding the virtual environment

agriyakhetarpal

Looks good to me, thanks for working on this, @akhilender-bongirwar! Tested this locally and it works without issues. I will request a review from @Saransh-cpp in case there are any additional changes to be made and also to get a workflow run triggered here

noxfile.py

- Added external = True instead of silent = False as it is not required for session.run()

akhilender-bongirwar · 2023-10-08T11:06:20Z

Looks good to me, thanks for working on this, @akhilender-bongirwar! Tested this locally and it works without issues. I will request a review from @Saransh-cpp in case there are any additional changes to be made and also to get a workflow run triggered here

Thank you, @agriyakhetarpal, for allowing me to work under your guidance for this issue. Although I made a few mistakes in the beginning, it has been great working together.

Saransh-cpp

Thanks for working on this, @akhilender-bongirwar! I think we'll still need to set the env variables before installing scikits.odes (the dev session fails for me locally) - @arjxn-py @agriyakhetarpal.

Also, we should use an if-else block to save time by installing pybamm only once -

noxfile.py

agriyakhetarpal · 2023-10-08T12:12:04Z

Okay, that's strange because it works well on my end (I checked and purged the cached wheels so that it builds a wheel for scikits.odes again). The dev session is meant to be run after pybamm-requires so that the SUNDIALS installation can be found (the set_environment_variables function is taking care of the SUNDIALS_INST environment variable)

agriyakhetarpal · 2023-10-08T12:17:58Z

Note: the earlier export command was incorrect anyway because it didn't change anything in the activation script, so if we want to keep that we should modify it to use a command similar to what pybamm_install_odes does

It might also be a discrepancy in the Python version. This newer dev session will create a virtualenv with the version of Python with which it was installed

Saransh-cpp · 2023-10-08T12:30:33Z

Weird. I'll suggest that we install scikits.odes only on Linux in this PR, given that we install scikits.odes only on Linux in the other nox sessions (except for the coverage session, don't really know why). #3417 can then modify the noxfile appropriately to support MacOS.

codecov · 2023-10-08T12:36:37Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (d02a78b) 99.58% compared to head (7629414) 99.58%.
Report is 53 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3408   +/-   ##
========================================
  Coverage    99.58%   99.58%           
========================================
  Files          256      256           
  Lines        19998    19998           
========================================
  Hits         19915    19915           
  Misses          83       83

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

agriyakhetarpal · 2023-10-08T12:51:49Z

Weird. I'll suggest that we install scikits.odes only on Linux in this PR

No worries, happy to help debug your installation offline 😄

except for the coverage session, don't really know why

This is because we don't compile the IDAKLU component for macOS in CI, because we are running pybamm-requires only on Linux. I have fixed that in #3301, but as discussed in the developer meeting we'll be merging that one after 23.9 non-RC is out, and that could take some time (the wheels have to be fixed too). @arjxn-py, please feel free to copy the relevant changes from there if you want to or create a new PR because that one is slightly big (but contains some needed changes).

akhilender-bongirwar · 2023-10-10T05:42:46Z

@agriyakhetarpal, should I still make any new changes or is the code working fine ?

agriyakhetarpal · 2023-10-10T09:32:41Z

For now, you may install [odes] only on Linux and also resolve Saransh's comments. Happy to approve then

- Removed redundant code Co-authored-by: Saransh Chopra <[email protected]>

- Added all the installation in a single line Co-authored-by: Saransh Chopra <[email protected]>

- Added else block for efficient running of the code Co-authored-by: Saransh Chopra <[email protected]>

noxfile.py

- Altered and removed the darwin platform

agriyakhetarpal

Everything looks good to me now. Thanks for working on this, @akhilender-bongirwar!

noxfile.py

- Made a single line of 89 characters into few smaller lines

agriyakhetarpal self-requested a review October 4, 2023 22:04

agriyakhetarpal reviewed Oct 5, 2023

View reviewed changes

noxfile.py Outdated Show resolved Hide resolved

noxfile.py Outdated Show resolved Hide resolved

noxfile.py Outdated Show resolved Hide resolved

noxfile.py Outdated Show resolved Hide resolved

akhilender-bongirwar requested review from arjxn-py and agriyakhetarpal October 5, 2023 18:37

agriyakhetarpal reviewed Oct 5, 2023

View reviewed changes

noxfile.py Outdated Show resolved Hide resolved

noxfile.py Outdated Show resolved Hide resolved

noxfile.py Outdated Show resolved Hide resolved

akhilender-bongirwar added 2 commits October 6, 2023 15:05

Merge branch 'develop' of github.com:akhilender-bongirwar/PyBaMM into…

d2b51ff

… enhancement/activate-venv-dev-nox

akhilender-bongirwar requested a review from agriyakhetarpal October 7, 2023 13:25

agriyakhetarpal reviewed Oct 7, 2023

View reviewed changes

noxfile.py Outdated Show resolved Hide resolved

agriyakhetarpal reviewed Oct 7, 2023

View reviewed changes

noxfile.py Outdated Show resolved Hide resolved

agriyakhetarpal requested changes Oct 7, 2023

View reviewed changes

akhilender-bongirwar requested a review from agriyakhetarpal October 7, 2023 15:21

akhilender-bongirwar force-pushed the enhancement/activate-venv-dev-nox branch from a5392dc to 713da15 Compare October 8, 2023 08:25

akhilender-bongirwar added 2 commits October 8, 2023 14:16

fix: Resolved indentaions

eae0bbb

- Corrected the space between virtualenv and cmake - Changed macos to darwin

Updated install-from-source.rst

570b96f

- Updated the docs regarding the virtual environment

agriyakhetarpal approved these changes Oct 8, 2023

View reviewed changes

noxfile.py Outdated Show resolved Hide resolved

noxfile.py Outdated Show resolved Hide resolved

agriyakhetarpal requested a review from Saransh-cpp October 8, 2023 10:45

fix: Changed few minor instances

9475395

- Added external = True instead of silent = False as it is not required for session.run()

Saransh-cpp requested changes Oct 8, 2023

View reviewed changes

noxfile.py Outdated Show resolved Hide resolved

noxfile.py Outdated Show resolved Hide resolved

noxfile.py Outdated Show resolved Hide resolved

akhilender-bongirwar and others added 3 commits October 11, 2023 15:54

Updated noxfile.py

eeb0856

- Removed redundant code Co-authored-by: Saransh Chopra <[email protected]>

Updated noxfile.py

3cdc937

- Added all the installation in a single line Co-authored-by: Saransh Chopra <[email protected]>

Updated noxfile.py

94e3fdc

- Added else block for efficient running of the code Co-authored-by: Saransh Chopra <[email protected]>

agriyakhetarpal requested changes Oct 11, 2023

View reviewed changes

noxfile.py Outdated Show resolved Hide resolved

Update noxfile.py

e8610dd

- Altered and removed the darwin platform

agriyakhetarpal approved these changes Oct 11, 2023

View reviewed changes

agriyakhetarpal reviewed Oct 12, 2023

View reviewed changes

noxfile.py Outdated Show resolved Hide resolved

akhilender-bongirwar and others added 2 commits October 13, 2023 12:00

Updated styles

f7e59ad

- Made a single line of 89 characters into few smaller lines

style: pre-commit fixes

7629414

akhilender-bongirwar requested a review from Saransh-cpp October 13, 2023 06:32

agriyakhetarpal mentioned this pull request Oct 17, 2023

Fix Windows unit and integration tests failures #3454

Merged

8 tasks

Saransh-cpp approved these changes Oct 18, 2023

View reviewed changes

Saransh-cpp merged commit ff8c998 into pybamm-team:develop Oct 18, 2023
25 of 33 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Implement Automatic Activation of Virtual Environment in the dev nox session #3408

fix: Implement Automatic Activation of Virtual Environment in the dev nox session #3408

akhilender-bongirwar commented Oct 4, 2023

agriyakhetarpal left a comment

agriyakhetarpal left a comment

agriyakhetarpal left a comment

agriyakhetarpal commented Oct 7, 2023

akhilender-bongirwar commented Oct 7, 2023

akhilender-bongirwar commented Oct 7, 2023

agriyakhetarpal commented Oct 7, 2023 •

edited

Loading

akhilender-bongirwar commented Oct 8, 2023

agriyakhetarpal left a comment

akhilender-bongirwar commented Oct 8, 2023

Saransh-cpp left a comment

agriyakhetarpal commented Oct 8, 2023

agriyakhetarpal commented Oct 8, 2023

Saransh-cpp commented Oct 8, 2023

codecov bot commented Oct 8, 2023 •

edited

Loading

agriyakhetarpal commented Oct 8, 2023

akhilender-bongirwar commented Oct 10, 2023

agriyakhetarpal commented Oct 10, 2023

agriyakhetarpal left a comment

fix: Implement Automatic Activation of Virtual Environment in the dev nox session #3408

fix: Implement Automatic Activation of Virtual Environment in the dev nox session #3408

Conversation

akhilender-bongirwar commented Oct 4, 2023

Description

Type of change

Key checklist:

Further checks:

agriyakhetarpal left a comment

Choose a reason for hiding this comment

agriyakhetarpal left a comment

Choose a reason for hiding this comment

agriyakhetarpal left a comment

Choose a reason for hiding this comment

agriyakhetarpal commented Oct 7, 2023

akhilender-bongirwar commented Oct 7, 2023

akhilender-bongirwar commented Oct 7, 2023

agriyakhetarpal commented Oct 7, 2023 • edited Loading

akhilender-bongirwar commented Oct 8, 2023

agriyakhetarpal left a comment

Choose a reason for hiding this comment

akhilender-bongirwar commented Oct 8, 2023

Saransh-cpp left a comment

Choose a reason for hiding this comment

agriyakhetarpal commented Oct 8, 2023

agriyakhetarpal commented Oct 8, 2023

Saransh-cpp commented Oct 8, 2023

codecov bot commented Oct 8, 2023 • edited Loading

Codecov Report

agriyakhetarpal commented Oct 8, 2023

akhilender-bongirwar commented Oct 10, 2023

agriyakhetarpal commented Oct 10, 2023

agriyakhetarpal left a comment

Choose a reason for hiding this comment

agriyakhetarpal commented Oct 7, 2023 •

edited

Loading

codecov bot commented Oct 8, 2023 •

edited

Loading