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.

Sign up for GitHub

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

Jump to bottom

config: suppress config deprecation warnings in compat mode #4829

Merged
merged 6 commits into from
Apr 27, 2022
Merged

config: suppress config deprecation warnings in compat mode #4829

merged 6 commits into from
Apr 27, 2022

Conversation

oliver-sanders
Copy link
Member

@oliver-sanders oliver-sanders commented Apr 20, 2022 •
edited by MetRonnie
Loading

Quickie spotted during support:

  • Configuration deprecation warnings are currently displayed in Cylc 7 compatibility mode.
  • Compatibility mode is expected to be used by Cylc 7/8 compatible workflows during this transition period.
  • If users were to follow these warnings they would make their workflows incompatible with Cylc 7.
  • This change suppresses these deprecation (and only deprecation) warnings in Cylc 7 compat mode.

Requirements 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.
  • Does not need tests (why?).
  • Appropriate change log entry included.
  • Documentation: Update docs re: validation cylc-doc#460

@oliver-sanders oliver-sanders added the could be better Not exactly a bug, but not ideal. label Apr 20, 2022
@oliver-sanders oliver-sanders added this to the cylc-8.0rc3 milestone Apr 20, 2022
@oliver-sanders oliver-sanders self-assigned this Apr 20, 2022
@oliver-sanders oliver-sanders force-pushed the deprecation-warnings-in-compat-mode branch 2 times, most recently from a0b7c0e to 88b880b Compare April 20, 2022 15:16
oliver-sanders
oliver-sanders commented Apr 20, 2022
View reviewed changes
cylc/flow/config.py
@@ -786,7 +786,6 @@ def _check_implicit_tasks(self) -> None:
not cylc.flow.flags.cylc7_back_compat
):
raise WorkflowConfigError(msg)
LOG.warning(msg)
Copy link
Member Author

Choose a reason for hiding this comment

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

This log message says "to allow implicit tasks ..." which isn't appropriate in Cylc 7 compat mode (as implicit tasks are already allowed by exception), if users followed this advice they would make the workflow Cylc 7 incompatible.

MetRonnie
MetRonnie approved these changes Apr 20, 2022
View reviewed changes
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.

I think this needs a docs update, I am happy to work on that

@MetRonnie

This comment was marked as resolved.

Sign in to view
MetRonnie
MetRonnie requested changes Apr 20, 2022
View reviewed changes
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.

Ah wait, will need to do something about

cylc-flow/cylc/flow/workflow_files.py

Lines 342 to 346 in 0c6af24

SUITERC_DEPR_MSG = (
f"Backward compatibility mode ON for CYLC 7 '{WorkflowFiles.SUITE_RC}'"
" files: please address deprecation warnings and upgrade to Cylc 8 graph"
f" syntax BEFORE renaming the file to '{WorkflowFiles.FLOW_FILE}'.\n"
)

cylc-flow/cylc/flow/cfgspec/workflow.py

Lines 108 to 113 in 88b880b

.. versionchanged:: 8.0.0
The configuration file was previously named ``suite.rc``, but that
name is now deprecated. Please take action on any deprecation
warnings before renaming ``suite.rc`` configuration files
to ``flow.cylc``.

@oliver-sanders oliver-sanders force-pushed the deprecation-warnings-in-compat-mode branch from 88b880b to a69e753 Compare April 20, 2022 17:17
@oliver-sanders
Copy link
Member Author

Ach dammit, sorry, should read things better.

@oliver-sanders
config: suppress config deprecation warnings in compat mode
4abc00a 
* Configuration deprecation warnings are currently displayed in Cylc 7
  compatibility mode.
* Compatibility mode is expected to be used by Cylc 7/8 compatible
  workflows during this transition period.
* If users were to follow these warnings they would make their workflows
  incompatible with Cylc 7.
* This change suppresses these deprecation (and only deprecation)
  warnings in Cylc 7 compat mode.
@oliver-sanders
Copy link
Member Author

FYI this changes the advice from "address warnings in compat mode" to "address warnings/errors after upgrading".

I think this makes more sense, it is hard (and possibly not really possible) to upgrade the graphing in compat mode because the graph execution logic is different?

@oliver-sanders oliver-sanders force-pushed the deprecation-warnings-in-compat-mode branch from a69e753 to 4abc00a Compare April 20, 2022 17:24
hjoliver
hjoliver approved these changes Apr 21, 2022
View reviewed changes
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Makes sense, but I think the back-compat mode warning should still explain how to upgrade. (A bigger warning is also a bit harder to ignore forever).

cylc/flow/workflow_files.py Outdated Show resolved Hide resolved
@MetRonnie
Copy link
Member

I have opened a PR against this PR: oliver-sanders#54

Is it worth warning about recurrence format 1 in Cylc 7 back compat mode, seeing as it's easy to do?

oliver-sanders and others added 3 commits April 21, 2022 10:11
@oliver-sanders @MetRonnie @hjoliver
Apply suggestions from code review
6a3f434 
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>
@oliver-sanders
Merge pull request #54 from MetRonnie/iso-fmt-1-warn
ba2c577 
Add warning about recurrence fmt 1 in Cylc7 back-compat mode
@oliver-sanders
Merge branch 'master' into deprecation-warnings-in-compat-mode
163bda0
@MetRonnie MetRonnie mentioned this pull request Apr 21, 2022
Merged
1 task
@MetRonnie
Copy link
Member

flake8 failing

./cylc/flow/workflow_files.py:343:80: E501 line too long (90 > 79 characters)
./cylc/flow/workflow_files.py:344:80: E501 line too long (88 > 79 characters)

@oliver-sanders
Copy link
Member Author

sorted (squash-merge me)

@MetRonnie MetRonnie merged commit b22fccd into cylc:master Apr 27, 2022
@oliver-sanders oliver-sanders deleted the deprecation-warnings-in-compat-mode branch April 27, 2022 13:10
@MetRonnie MetRonnie mentioned this pull request May 4, 2022
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MetRonnie MetRonnie MetRonnie approved these changes

@hjoliver hjoliver hjoliver approved these changes

Assignees

@oliver-sanders oliver-sanders

Labels
could be better Not exactly a bug, but not ideal.
Projects
None yet
Milestone
cylc-8.0rc3
Development

Successfully merging this pull request may close these issues.
3 participants
@oliver-sanders @MetRonnie @hjoliver