Skip to content

Update reportengine dependencies#61

Merged
scarlehoff merged 6 commits intomasterfrom
scarlehoff-patch-1
Mar 7, 2024
Merged

Update reportengine dependencies#61
scarlehoff merged 6 commits intomasterfrom
scarlehoff-patch-1

Conversation

@scarlehoff
Copy link
Copy Markdown
Member

@scarlehoff scarlehoff commented Nov 24, 2023

This PR does:

  • updates the ruamel and dask dependencies

  • Modify tests in test_executor.py and test_builder.py so as to be able to accept dask futures.

@scarlehoff scarlehoff changed the title Update dask dependency Update reportengine dependencies Nov 24, 2023
@scarlehoff
Copy link
Copy Markdown
Member Author

I will merge this by the time NNPDF/nnpdf#1861 is implemented.

If someone (@comane I guess is the only person with experience here) can review before that so much the better.

Since this is only dealing with a few test and dependencies and the latest version is already being used via conda, I guess it would be fine.

@RoyStegeman
Copy link
Copy Markdown
Member

@comane when you have time, could you please review this?



def _test_ns(self):
def _test_ns(self, promise=False):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a particular reason for calling this promise?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think it looked like a js promise objet to me

Copy link
Copy Markdown
Member

@comane comane left a comment

Choose a reason for hiding this comment

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

Hi @scarlehoff, @RoyStegeman, I simply updated the description of the PR as this is not only updating the dependencies

@RoyStegeman
Copy link
Copy Markdown
Member

@scarlehoff could you add a comment explaining why you restricted the ruamel_yaml version?

@scarlehoff
Copy link
Copy Markdown
Member Author

One of the reasons this needed review was because I did the things that made the code work.

Including ruamel. This version worked, others didn't...

Comment thread pyproject.toml Outdated


def _test_ns(self):
def _test_ns(self, promise=False):
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think it looked like a js promise objet to me

@scarlehoff
Copy link
Copy Markdown
Member Author

@comane are you fine with me merging this?

afaik you are the only one using the newer versions of validphys

Comment thread pyproject.toml
Comment on lines +27 to 28
"dask[distributed]",
]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
"dask[distributed]",
]
"dask[distributed]",
"bokeh!=3.0.*,>=2.4.2",
]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

is bokeh a required dependency? I don't see it anywhere

@comane
Copy link
Copy Markdown
Member

comane commented Mar 7, 2024

Hi @scarlehoff, I tested this branch with the most up to date version of validphys (nnpdf), by pip installing this branch.
I runned some example scripts (plot pdfs and data theory comparison) and they worked well even with dask.

I guess that the idea is to change the nnpdf dependency as well since right now it is checking a fixed commit (which also needs curio).

I added a suggestion for bokeh. This way one gets the dask dashboard.

@scarlehoff
Copy link
Copy Markdown
Member Author

I see. I'll add bokeh then as an extra. Since dependencies of reportengine affect nnpdf as well I'd rather not add many.

I guess that the idea is to change the nnpdf dependency as well since right now it is checking a fixed commit (which also needs curio).

Yes. At least for python 3.12.
But the first step is to create a conda package for this (since that's the blocker right now)

@scarlehoff scarlehoff merged commit f393ee9 into master Mar 7, 2024
@scarlehoff scarlehoff deleted the scarlehoff-patch-1 branch March 7, 2024 10:48
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