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

Issue617 refactor service with history 2 #652

Open
wants to merge 253 commits into
base: master
Choose a base branch
from

Conversation

dhblum
Copy link
Collaborator

@dhblum dhblum commented Jul 1, 2024

This is for #617 and #671 and in place of #622. It does everything from #622, but merges the Service architecture in a way to include the nrel/boptest-service git history.

kbenne and others added 30 commits September 24, 2020 09:55
Working (although incomplete) twoday example
Update API and add example controller
There is global attribution at the project root
Use redis messages to generate results as they are requested, instead of
the very time consuming approach of computing them on every step
Remove some unwanted Alfalfa isms
The exceptions are the min/max test, plus julia and javascript tests
@kbenne
Copy link
Contributor

kbenne commented Oct 28, 2024

My setup is similar to @EttoreZ's and I'm dissapointed that I cannot reproduce @javiarrobas's issue. My test system is a M1 MacBook Pro with macOS version 15.0.1. This computer did not have Docker so I downloaded and installed the latest Docker Desktop for Apple Silicon. That gave me Docker Engine 27.2.0, build 3ab4256. All of the test cases provisioned fine in my case.

@kbenne
Copy link
Contributor

kbenne commented Oct 28, 2024

@javiarrobas since I can't reproduce I'm trying to think of ways to help troubleshoot. One key thing is that the provision container uses a volume to bring the test case files into the container. I wonder if there is some stale data in there. Here's some possible debug steps.

  1. Run the provision container and look around. What files do you see?
docker compose run provision bash
ls testcases
  1. Make sure that all of the old volumes get destroyed.
docker compose down -v

@dhblum
Copy link
Collaborator Author

dhblum commented Oct 29, 2024

Thanks @kbenne for the additional testing. In some ways, good there were no issues, however, I would like to figure out what's going on here before merging. @javiarrobas if you can, please see if Kyle's suggestions help? I would emphasize that the use of docker compose down is the best, most complete, and preferred way of shutting down the BOPTEST service to ensure there aren't hanging elements that could impact subsequent deployments.

@javiarrobas
Copy link
Contributor

I finally could figure out what was going on. The suggestion of @kbenne turned out helpful for that. When running the provision container and inspecting the files I got:
image
which made me realize that the test_multizone_residential_hydronic was an empty folder located in my testcases' directory probably as a residual of something I did in the past. This was somehow preventing the provisioning of other test cases. Notice that while the folders were empty, it had the doc and models/wrapped.fmu subfolders inside:
image
We might want to implement some type of safety mechanism to provision only specific test cases from a list.

@kbenne
Copy link
Contributor

kbenne commented Oct 31, 2024

I think we can probably come up with a simple enhancement to the globbing mechanism that is used to find FMUs and make this more robust.

@kbenne
Copy link
Contributor

kbenne commented Oct 31, 2024

Or yeah.. Like you said @javiarrobas, we could just explicitly enumerate them in a list.

@dhblum
Copy link
Collaborator Author

dhblum commented Oct 31, 2024

Huzzah! 🎉 Thank you @javiarrobas for continuing to try. One thing I'm curious about is still why it got stuck if there was a valid .fmu in that directory? Regarding the point of explicit list, we may think about if that list should be in any way accessible so anyone who wants to load in their own test case when running on their machine/setup can. But that goes back to my initial question, I wonder why it got stuck.

@javiarrobas
Copy link
Contributor

One thing I'm curious about is still why it got stuck if there was a valid .fmu in that directory?

That's because it was not a valid .fmu file but an empty folder called wrapped.fmu. Don't ask me why it was there though because I don't know 😅

@dhblum
Copy link
Collaborator Author

dhblum commented Nov 1, 2024

Ohh ok, I see!

@dhblum
Copy link
Collaborator Author

dhblum commented Nov 6, 2024

@javiarrobas @kbenne @EttoreZ I've addressed all Javier's comments (including a proposed readme convergence), Kyle's implemented updates from ADRENALIN, and Ettore's helped with updating some utility scripts to work with the new architecture.

I think this PR is very very close to being ready to merge. Nothing more comes to my mind to be done at this point, except for a protection that was mentioned against what happened with Javier, but perhaps we save that for a specific issue on it's own as an enhancement?

Can you all review/test a final time(s) and let me know any further comments or issues to be addressed? If you're ok with merging, please approve the github review. Note that I expect pending tests to pass.

Copy link
Contributor

@javiarrobas javiarrobas left a comment

Choose a reason for hiding this comment

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

Looking good! I agree to address the safeguard mechanism for the issue I had in a separate issue.

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.

7 participants