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

platforms: use processed workflow config #302

Merged
merged 1 commit into from
Mar 19, 2024

Conversation

oliver-sanders
Copy link
Member

  • When retrieving the platform used by a task, use the processed workflow config rather than processing the config from scratch.
  • This avoids the need to access the DB to extract template variables (workflow params) which could cause public DB locking issues.

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • CHANGES.md entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@oliver-sanders oliver-sanders added the bug Something isn't working label Mar 18, 2024
@oliver-sanders oliver-sanders added this to the 1.3.3 milestone Mar 18, 2024
@oliver-sanders oliver-sanders self-assigned this Mar 18, 2024
@oliver-sanders
Copy link
Member Author

oliver-sanders commented Mar 18, 2024

Kicked off Rose tests against this branch: https://github.com/metomi/rose/actions/runs/8325392017

Not playing ball because of lint tests, reviewers will need to check manually, note, 2.1.x branch. Suggest the following:

t/rose-task-run/04-run-path-empty.t
t/rose-task-run/-app-fcm-make.8

Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

  • Read code change
  • Checked where code is likely to touch the database using ./t/rose-task-run/04-run-path-empty
  • Run test manually.

cylc/rose/platform_utils.py Outdated Show resolved Hide resolved
* When retrieving the platform used by a task, use the processed
  workflow config rather than processing the config from scratch.
* This avoids the need to access the DB to extract template variables
  (workflow params) which could cause public DB locking issues.
Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

Re-ran tests

@MetRonnie MetRonnie merged commit cefb5de into cylc:1.3.x Mar 19, 2024
7 checks passed
@oliver-sanders oliver-sanders deleted the flow-processed branch March 19, 2024 14:05
@hjoliver
Copy link
Member

hjoliver commented Mar 28, 2024

I think this will fix a bug we've just run into at NIWA. We have a custom Python module that determines Slurm accounting code from a project code in the environment. The fcm_make Rose app is failing when it parses the flow.cylc to get platforms info, because the PYTHONPATH to the project code module isn't getting through to that process (even though it works fine for Cylc at scheduler start-up).

@oliver-sanders
Copy link
Member Author

That makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants