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

Prefill templates #3147

Closed
wants to merge 8 commits into from
Closed

Prefill templates #3147

wants to merge 8 commits into from

Conversation

Oglopf
Copy link
Contributor

@Oglopf Oglopf commented Oct 30, 2023

Fixes #2973

@Oglopf Oglopf marked this pull request as draft October 30, 2023 18:18
@Oglopf
Copy link
Contributor Author

Oglopf commented Oct 30, 2023

Weird. These are saving as JSON in data/batch_connect/sys/bc_osc_paraview/prefill_templates/.

session_contexts_controller.rb#save_template is currently being used, yank that out since we don't want the json but instead yaml and replace with module behavior as well.

@Oglopf
Copy link
Contributor Author

Oglopf commented Oct 31, 2023

Ran into another issue. These templates will just populate without regard for what app they are in currently. So, prefill settings for paraview will show up for jupyter, etc.

I need to check if this was true before and see how they handled it or make a filter for that.

@Oglopf
Copy link
Contributor Author

Oglopf commented Nov 1, 2023

Plan is to just add another layer for the app names to hold the respective data in the prefill_template section.

@Oglopf
Copy link
Contributor Author

Oglopf commented Nov 2, 2023

was really confused with an issue until i realized this is currently setup to do some amount of saving in the controller, ,then more in the model.

yank the saving logic from the controller for the templates as a controller shouldn't be managing this logic.

Let the usersettingstore handle the sanitization of the safe_name and the updated_settings.

@Oglopf
Copy link
Contributor Author

Oglopf commented Nov 2, 2023

I've got myself a bit turned around. Is the save_templates supposed to be for other templates in the future or for the prefills?

I'm also a bit confused if the json is supposed to be totally converted to some type of yaml, or is the idea yaml with keys that have json objects?

@Oglopf
Copy link
Contributor Author

Oglopf commented Jan 3, 2024

The data structure as I understood what was asked:

---
prefill_templates:
  apps:
    - juypter:
        - test3: '{"cluster":"pitzer","account":"PZS0714","mode":"1","bc_num_hours":"1","node_type":"any","cuda_version":"cuda/8.0.44","num_cores":"1","version":"app_jupyter/3.1.18","bc_email_on_started":"0"}'
        - test4: '{"cluster":"pitzer","account":"PZS0714","mode":"1","bc_num_hours":"1","node_type":"any","cuda_version":"cuda/8.0.44","num_cores":"2","version":"app_jupyter/3.1.18","bc_email_on_started":"0"}'
    - paraview:
        - test2: '{"version":"paraview/5.8.0","account":"PZS0714","bc_num_hours":"1"}'

@johrstrom
Copy link
Contributor

While I was hacking on this, this is what I came up with. I know for sure we'd want to cast it all to YAML instead of stringifying that json and using that as the value.

---
batch_connect_templates:
  sys/bc_osc_iqmol:
  - cluster: owens
    account: PZS0714
    bc_num_hours: '1'
    num_cores: '1'
    node_type: any
    ood_bc_template_name: new one
  - cluster: owens
    account: PZS0714
    bc_num_hours: '5'
    num_cores: '5'
    node_type: any
    ood_bc_template_name: 5s all day

@Oglopf
Copy link
Contributor Author

Oglopf commented Jan 4, 2024

I guess looking at the structure there I'm a bit confused. I thought this was going to hold more than just these batch connect prefill templates, which is why it wasn't a full fledged model and a concern instead? That was the only reason i had an apps category. I just want to check is all, what you have is much easier to accomplish if that's the structure we want.

@johrstrom
Copy link
Contributor

I thought this was going to hold more than just these batch connect prefill templates

Sure there are & will be more siblings to what you call prefill_templates, but no children other than this feature.

@Oglopf Oglopf closed this Jan 5, 2024
@Oglopf Oglopf deleted the prefill-templates branch January 5, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

save templates into the users' profile
3 participants