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

BHoM managed Python virtualenvs #101

Merged

Conversation

tg359
Copy link
Contributor

@tg359 tg359 commented Aug 17, 2022

NOTE: Depends on

BHoM/Python_Toolkit#85

Issues addressed by this PR

Closes #

Handling of Python environments via BHoM rationalised around a base "full" environment, and virtualenvs for any toolkit/project-specific environment that can be pre-defined with custom codebases via a referenced local-package added upon installation.

This PR creates a virtualenv with preinstalled commonly used Python ML packages. It also removes a lot of older code that is unused in an attempt to clean-up this toolkit ready for any newcomers to start adding aligned with the new Python management framework.

Test files

83.zip

Changelog

Additional comments

@tg359 tg359 added severity:medium Slows progress, but workaround is possible size:M Measured in hours status:do-not-merge For instance, test PR, requires further discussion, or dependant PRs not ready for merge type:external-api-changes Imposed changes, including from dependency across other BHoM repos labels Aug 17, 2022
@tg359 tg359 requested a review from FraserGreenroyd August 17, 2022 10:11
@tg359 tg359 self-assigned this Aug 17, 2022
@tg359 tg359 requested a review from alelom August 17, 2022 10:20
Copy link
Member

@alelom alelom left a comment

Choose a reason for hiding this comment

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

Looks great, still need to test it, minor changes requested

@tg359 tg359 requested a review from alelom August 19, 2022 13:26
alelom
alelom previously approved these changes Aug 25, 2022
Copy link
Member

@alelom alelom left a comment

Choose a reason for hiding this comment

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

Thanks for the code changes, code looks good to me.
I am approving on the basis of code changes and on the provided test file, see considerations done in BHoM/Python_Toolkit#85 (review), given that the test file is the same both PRs.

@tg359
Copy link
Contributor Author

tg359 commented Aug 26, 2022

@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented Aug 26, 2022

@tg359 to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check core
  • check null-handling
  • check serialisation
  • check versioning
  • check installer

There are 35 requests in the queue ahead of you.

@bhombot-ci
Copy link

bhombot-ci bot commented Aug 26, 2022

@tg359 just to let you know, I have provided a check-versioning result to this Pull Request as it was detected to be linked to other Pull Requests in a series. The comment which triggered this check came from @tg359 on Python_Toolkit

@bhombot-ci
Copy link

bhombot-ci bot commented Aug 26, 2022

@tg359 just to let you know, I have provided a check-installer result to this Pull Request as it was detected to be linked to other Pull Requests in a series. The comment which triggered this check came from @tg359 on Python_Toolkit

@bhombot-ci
Copy link

bhombot-ci bot commented Aug 26, 2022

@tg359 just to let you know, I have provided a check-versioning result to this Pull Request as it was detected to be linked to other Pull Requests in a series. The comment which triggered this check came from @tg359 on LadybugTools_Toolkit

@bhombot-ci
Copy link

bhombot-ci bot commented Aug 26, 2022

The check installer has already been run previously and recorded as a successful check. This check has not been run again at this time.

@tg359
Copy link
Contributor Author

tg359 commented Sep 9, 2022

@BHoMBot check compliance

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 9, 2022

@tg359 to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance

There are 44 requests in the queue ahead of you.

@tg359
Copy link
Contributor Author

tg359 commented Sep 9, 2022

@BHoMBot check compliance

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 9, 2022

@tg359 to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance

There are 72 requests in the queue ahead of you.

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 9, 2022

@tg359 fix requested for project compliance.

The errors with the CSProject (.csproj) files have been recorded as annotations on the checks tab.

I will apply the fixes to every case detailed on the checks tab with the exception of any references to the target framework. I am unable to provide fixes to the Target Framework automatically, these will need to be performed manually. If you want to perform the fixes in a different manner please resolve this manually and rerun the check.

If you are happy for me to go ahead and perform this action, please reply with:

@BHoMBot fix project file ref. 8267194587

Fixed csproj for versioning
@tg359
Copy link
Contributor Author

tg359 commented Sep 9, 2022

@BHoMBot check compliance

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 9, 2022

@tg359 to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 9, 2022

@tg359 just to let you know, I have provided a check-versioning result to this Pull Request as it was detected to be linked to other Pull Requests in a series. The comment which triggered this check came from @tg359 on Python_Toolkit

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 9, 2022

@tg359 just to let you know, I have provided a check-versioning result to this Pull Request as it was detected to be linked to other Pull Requests in a series. The comment which triggered this check came from @FraserGreenroyd on Python_Toolkit

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 9, 2022

@tg359 just to let you know, I have provided a check-versioning result to this Pull Request as it was detected to be linked to other Pull Requests in a series. The comment which triggered this check came from @tg359 on Python_Toolkit

1 similar comment
@bhombot-ci
Copy link

bhombot-ci bot commented Sep 9, 2022

@tg359 just to let you know, I have provided a check-versioning result to this Pull Request as it was detected to be linked to other Pull Requests in a series. The comment which triggered this check came from @tg359 on Python_Toolkit

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 9, 2022

@tg359 just to let you know, I have provided a check-installer result to this Pull Request as it was detected to be linked to other Pull Requests in a series. The comment which triggered this check came from @FraserGreenroyd on LadybugTools_Toolkit

@FraserGreenroyd
Copy link

@BHoMBot check core

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 9, 2022

@FraserGreenroyd to confirm, the following actions are now queued:

  • check core

There are 16 requests in the queue ahead of you.

Copy link

@FraserGreenroyd FraserGreenroyd left a comment

Choose a reason for hiding this comment

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

Approving based on @alelom review as commits since then have been versioning and compliance.

@FraserGreenroyd
Copy link

@BHoMBot check ready-to-merge

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 9, 2022

@FraserGreenroyd to confirm, the following actions are now queued:

  • check ready-to-merge

There are 60 requests in the queue ahead of you.

@FraserGreenroyd FraserGreenroyd removed the status:do-not-merge For instance, test PR, requires further discussion, or dependant PRs not ready for merge label Sep 9, 2022
@FraserGreenroyd FraserGreenroyd merged commit ec71146 into main Sep 9, 2022
@FraserGreenroyd FraserGreenroyd deleted the Python_Toolkit-#83-SimplificationAndExternalEnvironments branch September 9, 2022 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
severity:medium Slows progress, but workaround is possible size:M Measured in hours type:external-api-changes Imposed changes, including from dependency across other BHoM repos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants