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

Set TF-associated dependencies #1143

Merged
merged 3 commits into from
Mar 13, 2021
Merged

Set TF-associated dependencies #1143

merged 3 commits into from
Mar 13, 2021

Conversation

scarlehoff
Copy link
Member

@Zaharid
Copy link
Contributor

Zaharid commented Mar 12, 2021

Do you have a preference for 2.4 vs 2.3 specifically at the cost of a dubious pinning for gast?

@scarlehoff
Copy link
Member Author

scarlehoff commented Mar 12, 2021

For a stable environment I wouldn't mind. Although I think Roy's environment has the right gast and 2.4? My only worry is opt_einsum I mixed yours and Roy's.

@Zaharid
Copy link
Contributor

Zaharid commented Mar 12, 2021

Ok, @scarlehoff please confirm explicitly you are happy with @RoyStegeman 's.

@scarlehoff
Copy link
Member Author

I'll need to check (I'll do so later today) but from first inspection looks ok. The main change needed is that this PR will need to be merged and change nnpdf4 to point to that merge commit.

@Zaharid
Copy link
Contributor

Zaharid commented Mar 12, 2021

I don't think that is strictly required: The only change is the environment and that we are pinning explicitly, no?

@scarlehoff
Copy link
Member Author

Ah, I thought we wanted the env to be also (more or less) what you get when you do a clean "conda install nnpdf".

But in that the only changes are some specific versions you are right

@RoyStegeman
Copy link
Member

RoyStegeman commented Mar 12, 2021

It's not strictly required, and also the opt_einsum would cause a conflict with the environment, but I think we can bump the version number of the env without causing any new conflicts. Although I wouldn't know why we would want that.

The point is that this env installs tf 2.3.0 but has the opt_einsum requirement of tf2.4.1.

@scarlehoff
Copy link
Member Author

scarlehoff commented Mar 12, 2021

The same as gast, in the tf setup file is set to be greater than 3.3 and the default by conda is 3.1 (because for 3.3 conda-forge is needed)

Scratch this. I can't do it right now, can you change the packages to the 2.3 versions?

@wilsonmr
Copy link
Contributor

So does this PR now produce a package which is compatible with the environment in @RoyStegeman's comment?

#1126 (comment)

If so, what needs to be done to merge? Do we then need to re-tag?

@Zaharid
Copy link
Contributor

Zaharid commented Mar 12, 2021

@wilsonmr

I don't think that is strictly required: The only change is the environment and that we are pinning explicitly, no?

But at this point I really want @scarlehoff to confirm it is fine this time given how baldly I screwed up.

@wilsonmr
Copy link
Contributor

Ah ok, I was scanning the comments for very specific tokens and missed that.

@scarlehoff
Copy link
Member Author

Ok, the environment seem fine. After 20k of a global fit was still under 5 GB of RAM and the time is unchanged from my normal conda environment. The tests also pass, not sure what else to check, but if someone else has a look it would be nice.

@Zaharid
Copy link
Contributor

Zaharid commented Mar 12, 2021 via email

@wilsonmr
Copy link
Contributor

Ok I'm going to launch the closure fits with that env, at the very worst I'll just rerun them again again when we run into the next problem 😉 people seem to be getting impatient

@scarlehoff scarlehoff added the run-fit-bot Starts fit bot from a PR. label Mar 12, 2021
@github-actions
Copy link

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

@wilsonmr
Copy link
Contributor

wilsonmr commented Mar 13, 2021

Fit looks identical to when we merged #1081

see #1081 (comment)

@RoyStegeman
Copy link
Member

Right, but that's good, I didn't really expect this PR to affect the outcome of the fit. Or maybe I'm missing the point you're trying to make?

@wilsonmr
Copy link
Contributor

I was just saying it was successful!

@scarlehoff
Copy link
Member Author

The timings are also within minutes of each other so everything seems fine.

Right, but that's good, I didn't really expect this PR to affect the outcome of the fit. Or maybe I'm missing the point you're trying to make?

better safe than sorry!

@Zaharid
Copy link
Contributor

Zaharid commented Mar 13, 2021

Good. Please merge this.

@scarlehoff scarlehoff merged commit 8ea5671 into master Mar 13, 2021
@scarlehoff scarlehoff deleted the conda_tensorflow_deps branch March 13, 2021 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-fit-bot Starts fit bot from a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants