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

Switch from lattice.dispatch to ct.dispatch #55

Merged
merged 27 commits into from
Feb 3, 2022
Merged

Conversation

kessler-frost
Copy link
Member

@kessler-frost kessler-frost commented Jan 31, 2022

  • I have added the tests to cover my changes.
  • I have updated the documentation, VERSION, and CHANGELOG accordingly.
  • I have read the CONTRIBUTING document.

Resolves #48

@kessler-frost kessler-frost self-assigned this Jan 31, 2022
@codecov
Copy link

codecov bot commented Jan 31, 2022

Codecov Report

Merging #55 (c78a967) into develop (af1f628) will increase coverage by 0.10%.
The diff coverage is 72.38%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #55      +/-   ##
===========================================
+ Coverage    71.79%   71.90%   +0.10%     
===========================================
  Files           23       27       +4     
  Lines         1468     1502      +34     
===========================================
+ Hits          1054     1080      +26     
- Misses         414      422       +8     
Impacted Files Coverage Δ
covalent/_workflow/lattice.py 60.00% <0.00%> (-9.12%) ⬇️
covalent_dispatcher/_core/execution.py 66.44% <66.44%> (ø)
covalent/_dispatcher_plugins/base.py 84.61% <84.61%> (ø)
covalent/__init__.py 89.47% <100.00%> (+1.23%) ⬆️
covalent/_dispatcher_plugins/__init__.py 100.00% <100.00%> (ø)
covalent/_dispatcher_plugins/local.py 100.00% <100.00%> (ø)
covalent/executor/base.py 47.86% <100.00%> (-1.73%) ⬇️
covalent_dispatcher/_core/__init__.py 100.00% <100.00%> (+33.78%) ⬆️
covalent/_shared_files/utils.py 79.00% <0.00%> (-1.00%) ⬇️
covalent/_results_manager/results_manager.py 93.10% <0.00%> (+1.72%) ⬆️

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 af1f628...c78a967. Read the comment docs.

@kessler-frost kessler-frost marked this pull request as draft February 1, 2022 06:12
@kessler-frost
Copy link
Member Author

Only thing left to update is the website. rst files, readme, and contributing has also been updated.

@kessler-frost
Copy link
Member Author

Will need to run the how tos, tutorials again to check if they are still working. Tests seem to be passing which take how-tos into account.

@kessler-frost
Copy link
Member Author

The only place its not updated is the website.

@kessler-frost kessler-frost marked this pull request as ready for review February 1, 2022 19:05
@santoshkumarradha
Copy link
Member

The only place its not updated is the website.

@kessler-frost can you slack me once this is merged and in pip ? I will update it there.

Copy link
Member

@santoshkumarradha santoshkumarradha left a comment

Choose a reason for hiding this comment

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

Great job @kessler-frost ! 🔥

    dispatcher=ct.dispatch(run_experiment)
    id=dispatcher(*params)

Only suggestion -

Using nested functions is an advanced pattern and not a beginner to intermediate friendly pattern. Could you change user facing example (atleast in read me) to a pattern as above where it is a bit more easier to digest than nested calls like func()() ?

Copy link
Member

@FyzHsn FyzHsn left a comment

Choose a reason for hiding this comment

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

I have added a couple of (non-blocking) questions that require some clarification. Also, for the sake of good practise, is it possible to add some unit tests for base and local.py? I know this is a huge issue already.

CONTRIBUTING.md Show resolved Hide resolved
tests/functional_tests/serialization_test.py Show resolved Hide resolved
tests/functional_tests/serialization_test.py Show resolved Hide resolved
@FyzHsn
Copy link
Member

FyzHsn commented Feb 3, 2022

@kessler-frost Looks great! Just had a couple of questions and I think it would be good practise to write some unit tests for base and local.py.

@kessler-frost
Copy link
Member Author

@FyzHsn I agree, I'll add some tests for BaseDispatcher and LocalDispatcher.

@FyzHsn
Copy link
Member

FyzHsn commented Feb 3, 2022

@FyzHsn I agree, I'll add some tests for BaseDispatcher and LocalDispatcher.

I really don't want to be the tests guy. But somehow, I can never escape it.

Copy link
Member

@FyzHsn FyzHsn left a comment

Choose a reason for hiding this comment

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

(non-blocking) I'd create an issue to add more in-depth unit tests for the wrappers in the local dispatcher. Also, you might want to put in Deprecation warning like @santoshkumarradha mentioned to give users a chance to adapt and not have all their code break suddenly. However, covalent has only been OS for a week, so it might be fine.

Syntax needs to be updated in concepts.rst.

FyzHsn
FyzHsn previously approved these changes Feb 3, 2022
Copy link
Member

@FyzHsn FyzHsn left a comment

Choose a reason for hiding this comment

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

This is good to be merged (depending on @santoshkumarradha 's thoughts on the deprecation).

Copy link
Member

@santoshkumarradha santoshkumarradha left a comment

Choose a reason for hiding this comment

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

💯 !!

Copy link
Member

@santoshkumarradha santoshkumarradha left a comment

Choose a reason for hiding this comment

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

THE BEST ! 🔥 🚀 🤼‍♀️

@kessler-frost kessler-frost merged commit d863723 into develop Feb 3, 2022
@kessler-frost kessler-frost deleted the switch-dispatch branch February 3, 2022 18:54
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.

Switch from workflow.dispatch to covalent.dispatch
3 participants