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

Bugfix/sof 5578 wip - Make Colab Great Again #71

Merged
merged 90 commits into from
Jun 21, 2023
Merged

Conversation

timurbazhirov
Copy link
Member

No description provided.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

requirements-colab.txt Outdated Show resolved Hide resolved
@mrakitin
Copy link
Contributor

TODO: before merging, run the following command:

$ scripts/change-branch-in-urls.sh bugfix/SOF-5578-WIP dev

Just a reminder for myself and reviewers that the command above needs to be run to fix the branch before merging to dev.

Update the other files which weren't mentioned in the PR review:

- README.md
- examples/job/create_and_submit_job.*
- examples/job/run-simulations-and-extract-properties.*
- examples/material/api_interoperability_showcase.*
- examples/material/create_material.*
- examples/material/get_materials_by_formula.*
- examples/material/import_materials_from_materialsproject.*
- examples/material/import_materials_from_poscar.*
- examples/workflow/get_workflows.*
@mrakitin mrakitin requested review from azech-hqs and chu-k June 20, 2023 02:36
@@ -89,5 +122,6 @@ def execute():
set_notebook_environment(environment_variables_config)


execute()
os.environ.update({"is_setup_executed": "True"})
if __name__ == "__main__":
Copy link

Choose a reason for hiding this comment

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

Minor thing, initialize_settings.py isn't being executed directly anymore in notebooks, we could remove it altogether

Copy link
Member Author

Choose a reason for hiding this comment

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

@mrakitin - should we remove it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for your review, @chu-k!

I defined a couple of functions that are used in CLI and in some notebooks, but not the old execute() one. I can move those to __init__.py if people are OK with it, and then the initialize_settings.py can be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's remove unused functions and rename this file to notebook.py maybe?

Copy link

@chu-k chu-k left a comment

Choose a reason for hiding this comment

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

Looks good

@mrakitin
Copy link
Contributor

All review comments have been addressed, ready for final review, before I update the branch (#71 (comment)).

@timurbazhirov
Copy link
Member Author

This can be merged

Copy link

@azech-hqs azech-hqs left a comment

Choose a reason for hiding this comment

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

looks good

@mrakitin mrakitin merged commit 5ad8b8b into dev Jun 21, 2023
2 checks passed
@mrakitin mrakitin deleted the bugfix/SOF-5578-WIP branch June 21, 2023 22:00
@mrakitin
Copy link
Contributor

Merged, thanks, All!

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.

None yet

4 participants