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

Update code to support China instance of the SB platform #11

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

aleksandarbaburski
Copy link
Collaborator

@aleksandarbaburski aleksandarbaburski commented May 13, 2024

Hi Ryan, we updated the code to support SB China Platform:

  • In the sevenbridges_platform.py, we added two child classes (SevenBridgesPlatform_US and SevenBridgesPlatform_CN) that inherit the functionality from SevenBridgesPlatform class, but have unique detect method and some platform configuration settings. Note: the detect method actually tries to connect to the US or CN platform because there is no other way to detect what platform the launcher is running on.
  • In regard to that, we updated SUPPORTED_PLATFORMS dict in init.py to include US and CN SB platforms.
  • platform_config.py file was added. It contains sb_credentials and sb_execution_settings, so those things are not hardcoded in the sevenbridges_platform.py

If you have any questions please let us know.

@golharam
Copy link

The platform_config.py settings are specific to SevenBridges. Can these be put inside the sevenbridges_platform.py file?

@aleksandarbaburski
Copy link
Collaborator Author

Yes, done!

class Config:
credentials = {
'US': {
'api_endpoint': 'https://bms-api.sbgenomics.com/v2',

Choose a reason for hiding this comment

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

This is a public repo....hard-coded urls should not be here. I think this is better passed in from the launcher/code using this repo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @golharam, yes, I see.
So, we should move credentials (api endpoint, token, profile) from this repo to launcher specific config files on github.com/bmsgh/ repos (RNA, 10X, WES...) and then, in launcher.py, provide those credentials as an input when run PAML code.
Is that correct?

Choose a reason for hiding this comment

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

yes, I thinks that the best thing to do.

Choose a reason for hiding this comment

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

credentials, tokens, etc should be provided from the launcher.

Copy link
Collaborator Author

@aleksandarbaburski aleksandarbaburski May 17, 2024

Choose a reason for hiding this comment

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

Hi @golharam,
I moved api endpoint, token and profile from this repo, meaning that those need be provided now as input to detect and connect methods when we call them. If we take RNA launcher.py for example those lines are going to look like this:
args.platform = PlatformFactory().detect_platform(credentials)
and
platform.connect(credentials)
where credentials is a dict (that will be defined in launcher config) with a following structure:

credentials = {
            'US': {
                'api_endpoint': '',
                'token': '',
                'profile': ''
            },
            'CN': {
                'api_endpoint': '',
                'token': '',
                'profile': ''
            }
        }

Let me know what you think.

@golharam golharam linked an issue May 15, 2024 that may be closed by this pull request
@aleksandarbaburski
Copy link
Collaborator Author

Hi @golharam, did you have chance to review last changes we made? Credentials are moved from this repo.

@aleksandarbaburski
Copy link
Collaborator Author

Hi @golharam,
I removed use_elastic_disk option from default execution_settings for china platform (as mentioned on the meeting).

Please review the changes and let me know (once you decide) whether we are going to store credentials here or on individual launcher repos (RNA, WES, etc). Based on that I'll make some adjustments to the code and finalise the #16 effort.

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.

Add support for SevenBridges China platform
2 participants