-
Notifications
You must be signed in to change notification settings - Fork 903
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
Fix/#874 - Block config and create version when create entities #878
Fix/#874 - Block config and create version when create entities #878
Conversation
- 38 FAILING tests
with self.__class__.__lock_version_is_initialized: | ||
self.__class__._version_is_initialized = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to unblock the configuration update, mainly for Notebook use-case, where the user wants to add or modify the Config after running the app for a while for testing.
We of course can only Config.unblock
here, but then when the Core service start again, the version will not be manage. This can be good or bad base on the use-case.
So the question here is that when re-start the Core service (in notebook), do we want to:
- clean all entities (manage version)
- keep all entities (only unblock the config)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get why you renamed input
and output
data node configs, but I am ok with the change.
""" | ||
Core._manage_version_and_block_config() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
""" | ||
# Check if the data node config has GLOBAL scope | ||
if config.scope is not Scope.GLOBAL: | ||
raise DataNodeConfigIsNotGlobal(config.id) | ||
|
||
Core._manage_version_and_block_config() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with patch("sys.argv", ["prog", "--production", "1.0"]): | ||
core = Core() | ||
core.run() | ||
assert caplog.text == "" | ||
core.stop() | ||
|
||
caplog.clear() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, this tests what happens when we run the Core service without any config.
Why do we need to remove this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of this test is to check if the command line has any error. However since we add some new log, it's hard to test this.
Also, this case has already been tested elsewhere in the version_cli test, so it's not needed here.
We just check for the fail cases now.
core = Core() | ||
core.run() | ||
assert caplog.text == "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we want to test the generated logs anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reason as above
Oh @jrobinAV, it's because "input" and "output" are attributes of a task, which should not be named for a dn config. In those tests, we did not check the Config before, and now we do. |
Oh, right. Mystery solved! |
Resolves #874
In Taipy 3.0, we only create the version entity and block Configuration update when starting the Core service.
This makes application that creating entities without running the Core service will have entities that doesn't have an actual version, which can be dangerous.
In this PR:
taipy.create_scenario
ortaipy.create_global_data_node
APIs.