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

Added SparkSession configs statements if non-default namespace used #118

Conversation

morooshka
Copy link
Contributor

Added SparkSession configs statements if non-default namespace used to the demo notebook. Otherwise the SparkSession would not be created with an error referring to the wrong namespace

@stackable-bot
Copy link

stackable-bot commented Nov 6, 2024

CLA assistant check
All committers have signed the CLA.

@lfrancke lfrancke requested a review from maltesander November 6, 2024 16:22
@lfrancke
Copy link
Member

lfrancke commented Nov 6, 2024

Thank you for this PR! We'll take a look at this in the next few days.

@maltesander
Copy link
Member

Hi @morooshka,
thanks for your contribution :)

The PR looks good overall, i will try to test tomorrow.

We would like to keep the output (charts and tables) in the notebook though. This way you can e.g. see what the results look like e.g. via github as well or if anything goes wrong (e.g. like not enough memory etc.) during execution.

Im fine with you doing it, otherwise i can push the executed notebook again including your changes.

Cheers,
Malte

@morooshka
Copy link
Contributor Author

Hi @morooshka, thanks for your contribution :)
The PR looks good overall, i will try to test tomorrow.
We would like to keep the output (charts and tables) in the notebook though. This way you can e.g. see what the results look like e.g. via github as well or if anything goes wrong (e.g. like not enough memory etc.) during execution.
Im fine with you doing it, otherwise i can push the executed notebook again including your changes.
Cheers, Malte

OK, I'll repush it. I thought someone had forgotten to delete the outputs
Thanks for quick responce

@morooshka
Copy link
Contributor Author

Hi @maltesander! I've repushed the notebook with outputs as requested

@maltesander
Copy link
Member

Hey @morooshka, tested and works. Thank you!

One nitpick, there is one typo deploynig when defining the namespace (i cannot suggest via git due to large diff).

# Modify this please if using non-default product namespace for demo deploynig 
NAMESPACE: str = "default"

Otherwise we can merge :)

@morooshka
Copy link
Contributor Author

Hey @morooshka, tested and works. Thank you!
One nitpick, there is one typo deploynig when defining the namespace (i cannot suggest via git due to large diff).

OMG, thank you for noticing it, I'll repush

@morooshka
Copy link
Contributor Author

@maltesander I have repushed, thank you

Copy link
Member

@maltesander maltesander left a comment

Choose a reason for hiding this comment

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

No worries and thank you, looks good to me :)

@maltesander maltesander added this pull request to the merge queue Nov 8, 2024
Merged via the queue into stackabletech:main with commit 86aa43d Nov 8, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

4 participants