Closed 88d52bdba0366127fffca9dfa93895 closed 1 year ago
@yosukesan Good point, absolutely agree with you.
Your changes are good, could you make a PR, maybe in this patch. Because the dockerfile is not a main file for this project, it focuses more on the cookbook
, therefore some hardcoded libraries are acceptable in this case. Hardcoded versions are not.
For an official dockerfile to build this library, we should have a minimal working dockerfile, maybe in the root folder, not in the docker
subfolder. That's quite easy, just simply remove the hardcoded libraries and clean the dockerfile. How do you think about this @robertmartin8
I never use docker, so I don't have opinion on this. Happy to defer to yours – let me know when this is good to be merged!
I'm going to send additional amendment to @88d52bdba0366127fffca9dfa93895's fix since current patch terminated with err when I tested.
Your changes are good, could you make a PR, maybe in this patch
I'm unfamiliar to send additional patch over a patch. Should I send to https://github.com/88d52bdba0366127fffca9dfa93895/PyPortfolioOpt/tree/patch-1 ?
@yosukesan Yes, sure. Please let me known when it is ready for review.
I just send a patch. Manually tested on my machine and all passed. See below for the detail. https://github.com/88d52bdba0366127fffca9dfa93895/PyPortfolioOpt/pull/1
@88d52bdba0366127fffca9dfa93895 if all looks good, I can merge. Let me know
LGTM. Many thanks @yosukesan @robertmartin8
Thanks @robertmartin8, but only @88d52bdba0366127fffca9dfa93895's patch was merged. I think @88d52bdba0366127fffca9dfa93895 didn't resend pull request after my patch was merged.
Could I make additional issue and send patch for it ? Probably easier to chase the change log later.
Sure
Sorry guys, I forgot to merge the patch. Thanks @yosukesan for your finding.
Sorry guys, I forgot to merge the patch. Thanks @yosukesan for your finding.
No worries. I didn't know git worked this way either.
@xtuanta Have you tested ?
Guest OS trying to use pip==21.0.1 and poetry==1.1.4". Then failed on my env. See Err Log. pip and poetry version are hard coded on docker/Dockerfile. I believe this is not good manner.
Since pyproject.toml and poetry.lock are copied to guest env and used. I think it is safe to just remove hard coded version. After I did so, docker image was successfully generated. See Normally terminated case.
Since @xtuanta already sent a patch, could I ask additional fix ? I'll manually test on your branch. Better to add docker build and test workflow as well, but probably better to make another issue for it.
I forgot to mention. buster is debian's old stable which has EOL next summer. Better to switch to current stable version.
Normally terminated case: Changes from robertmartin8's original
Err Log: Err termination by using pip==21.0.1 and poetry==1.1.4